Skip to content
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

Closed
wants to merge 14 commits into from

Conversation

Vishal2002
Copy link

@Vishal2002 Vishal2002 commented Mar 17, 2024

Description

Done.mp4

Copy link

netlify bot commented Mar 17, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 66fe5de
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/663249082a62150008a46699
😎 Deploy Preview https://deploy-preview-2790--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sambhavgupta0705
Copy link
Member

@Vishal2002 please the correct pr title convention

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Mar 17, 2024

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 45
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🟠 PWA 56

Lighthouse ran on https://deploy-preview-2790--asyncapi-website.netlify.app/

@Vishal2002 Vishal2002 changed the title Navigation to correct heading in tools section fix: Navigation to correct heading in tools section Mar 17, 2024
@Vishal2002
Copy link
Author

@Vishal2002 please the correct pr title convention

@sambhavgupta0705 Done Sir

@Vishal2002 Vishal2002 changed the title fix: Navigation to correct heading in tools section fix: navigation to correct heading in tools section Mar 17, 2024
@sambhavgupta0705
Copy link
Member

@Vishal2002 it doesn't solve the issue for us
As we need the url to be responsive to the request

@Vishal2002
Copy link
Author

@Vishal2002 it doesn't solve the issue for us As we need the url to be responsive to the request

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 useref and scroll into View to solve the issue.I did the same and solved it. Would be great if you give some more context about my mistake.

@sambhavgupta0705
Copy link
Member

ohhh my bad
I missed the preview you added 😅

Copy link
Member

@sambhavgupta0705 sambhavgupta0705 left a 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

components/tools/ToolsList.js Show resolved Hide resolved
components/tools/ToolsList.js Show resolved Hide resolved
@sambhavgupta0705
Copy link
Member

@Vishal2002 any update??

@Vishal2002
Copy link
Author

@Vishal2002 any update??

Sir already answered the question which you asked about hash and decodeHash.

@sambhavgupta0705
Copy link
Member

I can't see any reply
I think you haven't finished the review

@Vishal2002
Copy link
Author

I can't see any reply I think you haven't finished the review

Sorry, ya I failed to resolve that, I think now it will be visible to you.

@sambhavgupta0705
Copy link
Member

still cannot see
you marked them as resolved only

@Vishal2002
Copy link
Author

Vishal2002 commented Mar 30, 2024

still cannot see you marked them as resolved only

IDK why this is happening but I have commented on the question that you asked me before about hash and decodehash.

Screenshot 2024-03-30 160825
Screenshot 2024-03-30 160813

@akshatnema
Copy link
Member

IDK why this is happening but I have commented on the question that you asked me before about hash and decodehash.

@Vishal2002 I can see that your review is still pending. Inside the Files changed, click on the Review Changes button and click comment option there. Your review will be posted.

@Vishal2002
Copy link
Author

IDK why this is happening but I have commented on the question that you asked me before about hash and decodehash.

@Vishal2002 I can see that your review is still pending. Inside the Files changed, click on the Review Changes button and click comment option there. Your review will be posted.

Thank you @akshatnema Sir and @sambhavgupta0705 Sorry for the inconvenience.

categoryRef.scrollIntoView({ behavior: 'smooth', block: 'start' });
}
}
}, []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}, []);
}, []);

Copy link
Author

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 ?

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clean code rules

Copy link
Author

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(() => {
Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akshatnema Done Sir

@akshatnema
Copy link
Member

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

@anshgoyalevil
Copy link
Member

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement.
Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

Yes, it is there for me as well.

@Vishal2002
Copy link
Author

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement. Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

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?
https://drive.google.com/file/d/1haMMTh7psG3pICxCwDqKHd4IfwLBzZBD/view?usp=sharing

@anshgoyalevil
Copy link
Member

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement. Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

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? https://drive.google.com/file/d/1haMMTh7psG3pICxCwDqKHd4IfwLBzZBD/view?usp=sharing

In this video itself, you can see the extra scroll issue. From 00:00 to 00:15 seconds.

@Vishal2002
Copy link
Author

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement. Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

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? https://drive.google.com/file/d/1haMMTh7psG3pICxCwDqKHd4IfwLBzZBD/view?usp=sharing

In this video itself, you can see the extra scroll issue. From 00:00 to 00:15 seconds.

The additional scrolling occurs infrequently.

@sambhavgupta0705
Copy link
Member

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement. Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

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? https://drive.google.com/file/d/1haMMTh7psG3pICxCwDqKHd4IfwLBzZBD/view?usp=sharing

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
@Vishal2002 we need to fix it before merging this one

@Vishal2002
Copy link
Author

Can you please recheck it on your end and then we are good tyo merge this PR

There's still extra scroll on the desktop and mobile view. Have room for improvement. Try opening the link https://deploy-preview-2790--asyncapi-website.netlify.app/tools#Others

@sambhavgupta0705 @anshgoyalevil I find that there is extra scroll on mobile view. Can you check once?

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? https://drive.google.com/file/d/1haMMTh7psG3pICxCwDqKHd4IfwLBzZBD/view?usp=sharing

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 @Vishal2002 we need to fix it before merging this one

@sambhavgupta0705 Sir at my side it happens for the first time when the page renders and after that, everything go smoothly.

@anshgoyalevil
Copy link
Member

anshgoyalevil commented Apr 12, 2024

@Vishal2002

happens for the first time when the page renders

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

@Vishal2002
Copy link
Author

Vishal2002 commented Apr 14, 2024

@Vishal2002

happens for the first time when the page renders

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

@Vishal2002
Copy link
Author

@anshgoyalevil and @sambhavgupta0705 Sir looking for the reply from your side.

@sambhavgupta0705
Copy link
Member

@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
@sambhavgupta0705
Copy link
Member

@Vishal2002 the issue is still there

@sambhavgupta0705
Copy link
Member

@Vishal2002 please resolve the merge conflicts

@Vishal2002
Copy link
Author

Vishal2002 commented Jun 8, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links in tools section to specific category do not work
5 participants