fix: fixed links to specific category in tools section #2190
Conversation
aryanas159
requested review from
derberg,
akshatnema,
magicmatatjahu,
Mayaleeeee and
asyncapi-bot-eve
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-2190--asyncapi-website.netlify.app/ |
aryanas159
changed the title
akshatnema
changed the title
| element.scrollIntoView({ behavior: 'smooth', block: "nearest", inline: "nearest" }) | ||
| } | ||
| }, 1000) | ||
| }, []) |
There was a problem hiding this comment.
| }, []) | |
| }, []) | |
Kindly add a spacing to make it more readable inside codebase (since they are 2 different sections of code).
components/tools/ToolsList.js
Outdated
| @@ -6,7 +6,7 @@ export default function toolsList({ toolsData }) { | |||
| <div className="" data-testid="ToolsList-main" > | |||
| {Object.keys(toolsData).map((categoryName, index) => { | |||
| if(toolsData[categoryName].toolsList.length > 0) return ( | |||
| <div className='my-8' key={index} id={categoryName}> | |||
| <div className='my-8' key={index} id={categoryName} style={{scrollMargin: "100px"}}> | |||
There was a problem hiding this comment.
You can't use inline styles to add styling inside div. Kindly use tailwind utility classes to add the CSS.
scripts/build-newsroom-videos.js
Outdated
| @@ -26,6 +26,7 @@ async function buildNewsroomVideos() { | |||
| const videoData = JSON.stringify(videoDataItems, null, ' '); | |||
| console.log('The following are the Newsroom Youtube videos: ', videoData) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Kindly remove non-relevant changes from your PR.
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-2190--asyncapi-website.netlify.app/ |
| setTimeout(() => { | ||
| const path = router?.asPath | ||
| const id = path?.slice(path?.indexOf("#") + 1).replace("%20", " ") | ||
| const element = document.getElementById(id) |
There was a problem hiding this comment.
Instead of using id attribute, can't we use ref here to add the behavior?
There was a problem hiding this comment.
We can think about this, but the "Jump to Category" dropdown already uses the "id" of each div to scroll to, so wouldn't it be better to use the same strategy for initial scrolling as well?
There was a problem hiding this comment.
We usually access the DOM elements directly in react. React has it's own virtual DOM feature which enables better performance on the website. That's why I asked to use ref in this case. We used id just to reference the category and link it in dropdown.
components/tools/ToolsList.js
Outdated
| @@ -6,7 +6,7 @@ export default function toolsList({ toolsData }) { | |||
| <div className="" data-testid="ToolsList-main" > | |||
| {Object.keys(toolsData).map((categoryName, index) => { | |||
| if(toolsData[categoryName].toolsList.length > 0) return ( | |||
| <div className='my-8' key={index} id={categoryName}> | |||
| <div className='my-8 scroll-m-[100px]' key={index} id={categoryName}> | |||
There was a problem hiding this comment.
Don't use units in px. Instead use rem, em or tailwind utility classes to add a scroll margin. Using pixels will impact responsiveness of the website.
|
@aryanas159 are you still working on it |
|
@sambhavgupta0705 No I'm not, I put a reply above for the changes requested, but I don't think they were reviewed. |
There was a problem hiding this comment.
Why there are changes in this file?
Description
Untitled.video.-.Made.with.Clipchamp.4.mp4
Related issue(s)
Fixes #2181