-
-
Notifications
You must be signed in to change notification settings - Fork 681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: navigation to correct heading in tools section #2790
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@Vishal2002 please the correct pr title convention |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-2790--asyncapi-website.netlify.app/ |
@sambhavgupta0705 Done Sir |
@Vishal2002 it doesn't solve the issue for us |
But as per an actual issue by @derberg where he mentioned that when trying to navigate to https://www.asyncapi.com/tools#Documentation%20Generators he is unable to navigate to the actual heading.After that I also read the discussion where @akshatnema told a fellow contributor to use |
ohhh my bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vishal2002 I have checked the netlify preview it works fine
Just added some comments for better understanding
@Vishal2002 any update?? |
Sir already answered the question which you asked about |
I can't see any reply |
Sorry, ya I failed to resolve that, I think now it will be visible to you. |
still cannot see |
@Vishal2002 I can see that your review is still pending. Inside the |
Thank you @akshatnema Sir and @sambhavgupta0705 Sorry for the inconvenience. |
components/tools/ToolsList.js
Outdated
categoryRef.scrollIntoView({ behavior: 'smooth', block: 'start' }); | ||
} | ||
} | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, []); | |
}, []); | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What changes to add here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just add a newline at the end of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clean code rules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done Sir
import Paragraph from '../typography/Paragraph' | ||
export default function toolsList({ toolsData }) { | ||
const categoryRefs = useRef({}); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment, explaining about this useEffect
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It scrolls the page to a specific category referenced in the URL hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akshatnema Done Sir
@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once? |
There's still extra scroll on the desktop and mobile view. Have room for improvement.
Yes, it is there for me as well. |
I didn't find this mobile view issue, Sir please can you tell me how to produce this again? |
In this video itself, you can see the extra scroll issue. From 00:00 to 00:15 seconds. |
The additional scrolling occurs infrequently. |
The extra scroll is happening for me also |
@sambhavgupta0705 Sir at my side it happens for the first time when the page renders and after that, everything go smoothly. |
That's what we are targeting in this issue. An end user would click on a link that would render for the first time for them. They won't be stuffing the hash values to URLs manually after the first render |
@anshgoyalevil and @sambhavgupta0705 Sir there might be issue with height of the container and if we add height:100vh then it solves the issue. height-issue.mp4 |
@anshgoyalevil and @sambhavgupta0705 Sir looking for the reply from your side. |
@Vishal2002 push that change once to this branch and let us see the preview |
added container height to resolve extra scroll added container height to resolve extra scroll added height to resolve the extra scroll issue
…into scroll-issue
@Vishal2002 the issue is still there |
@Vishal2002 please resolve the merge conflicts |
@sambhavgupta0705 Ok and the previous issue regarding extra scroll might be because of useref unable to get the reference for the first time the component mounts. |
Description
useRef
hook to reference the target element, improving navigation reliability.Related issue(s)
Fixes: Links in tools section to specific category do not work #2181
Done.mp4