fix: links in tools section to specific category do not work #3116
Conversation
✅ Deploy Preview for asyncapi-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
JeelRajodiya
changed the title
JeelRajodiya
changed the title
| @@ -1,5 +1,4 @@ | |||
| import { useEffect, useRef, useState } from 'react'; | |||
| import TextTruncate from 'react-text-truncate'; | |||
There was a problem hiding this comment.
TextTruncate library caused layout shifts post-render, prevented to add consistent scroll padding. If I'd kept TextTruncate, I would need inconsistent scroll padding. The further down the tools page I went, the more padding I'd need for each element.
So yes, changes in this file are relevant to the issue.
| useEffect(() => { | ||
| setLoading(true); | ||
| setTimeout(() => { | ||
| setLoading(false); | ||
| }, 1000); | ||
| }, []); | ||
|
|
There was a problem hiding this comment.
I have removed this timeout and the loading state. Not sure why it was there before. Let me know if anything breaks because of this
components/tools/ToolsDashboard.tsx
Outdated
| useEffect(() => { | ||
| const { hash } = window.location; | ||
|
|
||
| if (hash) { | ||
| const elementID = decodeURIComponent(hash.slice(1)); | ||
| const element = document.getElementById(elementID); | ||
|
|
||
| if (element) { | ||
| document.documentElement.style.scrollPaddingTop = '6rem'; | ||
| element.scrollIntoView({ behavior: 'smooth' }); | ||
| document.documentElement.style.scrollPaddingTop = '0'; | ||
| } | ||
| } | ||
| }, []); | ||
| // Function to update the list of tools according to the current filters applied |
There was a problem hiding this comment.
The actual logic to scroll to the element. The padding is removed afterward so that it does not interfere with the other parts of the website
JeelRajodiya
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3116--asyncapi-website.netlify.app/ |
|
The PR is ready to review! |
|
@akshatnema PTAL |
|
@akshatnema @anshgoyalevil PTAL |
|
/update |
|
@akshatnema PTAL |
| const element = document.getElementById(elementID); | ||
|
|
||
| if (element) { | ||
| document.documentElement.style.scrollPaddingTop = '6rem'; |
There was a problem hiding this comment.
Instead of directly modifying the DOM, can't we use inbuilt hooks of react to interact with virtual DOM?
Modifying real DOM in react is a bad practice.
There was a problem hiding this comment.
Agree! We are now using react refs to focus the category divs (I have pushed the changes).
but for the scroll padding we need the body element, I will have to pass the ref of the of the body element to ToolDashboard page. Should I go ahead and do it as well?
| ref={descriptionRef} | ||
| className={`line-clamp-3 max-h-[4.5em] overflow-hidden text-ellipsis ${isTruncated && 'after:content-["..."]'}`} | ||
| > | ||
| {toolData.description} |
There was a problem hiding this comment.
How are we truncating the text here?
There was a problem hiding this comment.
The text is truncated using line-clamp-3, which limits the text to three lines. If isTruncated is true, the after:content-["..."] adds ellipsis (...) after the truncated text.
The styles max-h-[4.5em] overflow-hidden text-ellipsis are not relevant. I have removed them in the latest commit. Apologies for adding them initially.
| useEffect(() => { | ||
| updateToolsList(); | ||
| }, [isPaid, isAsyncAPIOwner, languages, technologies, categories, searchName]); | ||
|
|
There was a problem hiding this comment.
Why you removed this useEffect? It's important to apply filters on Tools dashboard.
There was a problem hiding this comment.
I have moved the filter logic into a useMemo hook (please check here). The purpose of using useMemo is to avoid storing the tools list in a React state. Initially, when we used React state, the tools list took some time to appear after the page loaded, which made scrolling difficult.
I can ensure that the filters are still being applied.
components/tools/ToolsDashboard.tsx
Outdated
| {loading ? ( | ||
| <div className='mx-auto my-24 flex w-fit animate-pulse gap-4 text-black'> | ||
| <img src={loader} alt='loading' className='mx-auto w-16' /> | ||
| <div className='my-auto text-xl'>Loading Tools...</div> | ||
| </div> | ||
| ) : ( |
There was a problem hiding this comment.
We want loading inside tools dashboard or lazy loading because there are too much content to show in the tools dashboard page. Not applying any preloader will give blank screen to the user, on first interaction with page.
There was a problem hiding this comment.
Akasht, Could you please visit the deployed version of this PR?
https://deploy-preview-3116--asyncapi-website.netlify.app/tools
We can keep the loading state, but based on the deployed version, I don't think it's necessary, as the content is instantly visible when the page loads.
There was a problem hiding this comment.
Enable throttling inside your browser and you will then see a long wait with a white screen.
There was a problem hiding this comment.
Hello @akshatnema, Thanks for the suggestion. I have tried enabling throttling, and yes I did see a long wait with a white screen. However, when I switched to the master branch, the long wait screen was still there. Please see the below recordings.
Browser: Chrome (incognito tab)
Throttling: Fast 4G
On the master branch - White screen for ~9.83s
on-Master.mp4
On the fix-scroll-id branch (branch of current PR) - White screen for ~10.38s
on-PR.mp4
When I measured the time multiple times, it differed between 9.5s to 10.5s on both of the branches.
|
@akshatnema PTAL |
|
@akshatnema PTAL |
|
@akshatnema PTAL |
|
/rtm |
Description
When you go to url (enter url directly in the browser) https://www.asyncapi.com/tools#Converters it will not scroll to the Converters element
This PR closes #2181
Deployed version of this PR: https://deploy-preview-3116--asyncapi-website.netlify.app/tools#Directories