-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(#1932): implement smooth scrolling for anchor navigation across site #1965
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
base: gh-pages
Are you sure you want to change the base?
feat(#1932): implement smooth scrolling for anchor navigation across site #1965
Conversation
✅ Deploy Preview for expressjscom-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
js/app.js
Outdated
$(document).on('click', 'a[href^="#"]:not([href="#"]):not([href="#top"])', function(e) { | ||
var href = $(this).attr('href'); | ||
if (smoothScrollToElement(href)) { | ||
e.preventDefault(); |
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.
Can you please remove this line https://github.com/expressjs/expressjs.com/blob/gh-pages/js/menu.js#L186 ? Since default behavior stopped here.
js/app.js
Outdated
// top link | ||
$('#top').click(function(e){ | ||
$('html, body').animate({scrollTop : 0}, 500); | ||
return false; | ||
}); | ||
|
||
// Smooth scrolling for all anchor links | ||
$(document).on('click', 'a[href^="#"]:not([href="#"]):not([href="#top"])', function(e) { |
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.
$(document).on('click', 'a[href^="#"]:not([href="#"]):not([href="#top"])', function(e) { | |
$("#api-doc").on('click', 'a[href^="#"]:not([href="#"]):not([href="#top"])', function(e) { |
We should minimize the scope of this click event. Currently, $(document).on('click'...) attaches the handler to all anchor links with #
across every page. However, smooth scrolling is only needed on API pages, since the TOC exists only there. Scoping it to #api-doc
avoids unnecessary event handling and prevents inconsistent anchor #
behavior on other pages. Also, checking "#api-doc"
exist on page is required before applying click event.
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.
Thanks for the PR!
As i mentioned in the issue, this can be handled through CSS, javascript is not necessary.
I am OK with CSS only solution. But this approach comes with some limitations, like we cannot control scroll behavior and can not add header offsets. But it is fine if headings are not hiding behind the header bar on anchor link click. |
Thanks @bjohansebas for the suggestion about a CSS-only solution!
So I’d propose keeping the JS smooth scroll, scoped only to Let me know if you’d prefer another approach! |
The headings are never hidden by the header bar, or at least I’ve been trying to reproduce it and haven’t been able to.
That doesn’t matter in CSS, since the effect will be applied to all elements anyway. * {
scroll-behavior: smooth;
} |
Hi @bjohansebas and @ShubhamOulkar, I’ve updated the implementation to follow your suggestion of a CSS-only smooth scrolling solution.
Let me know if you’d like any further changes! |
Description
This PR implements smooth scrolling for in-page navigation across the Express.js website as mentioned in Issue : #1932
This improves the user experience by ensuring that navigation to sections via anchor links transitions smoothly, rather than jumping abruptly.
Closes #1932
Video
smooth-scrolling.mp4