Skip to content

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 2, 2025
Merged

feat(#1932): implement smooth scrolling for anchor navigation across site #1965

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ee25a7b
feat(#1932): implement smooth scrolling for anchor navigation across …
Mohit5Upadhyay Jul 7, 2025
736831c
feat: implement CSS-only smooth scrolling for anchor navigation
Mohit5Upadhyay Jul 12, 2025
9ce8ec4
apply reviewer changes: Update scroll-margin-top to 60px and remove p…
Mohit5Upadhyay Jul 15, 2025
6daa170
Merge branch 'gh-pages' into feat-1932-smooth-scrolling
bjohansebas Aug 2, 2025
48aed9e
remove scroll to top functionality and prevent default navigation fo…
bjohansebas Aug 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions css/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,20 @@
* {
box-sizing: border-box;
-webkit-tap-highlight-color: transparent;
scroll-behavior: smooth;
}

/* handle header offset with scroll-margin */
h1, h2, h3, h4, h5, h6,
[id] {
scroll-margin-top: 120px;
Copy link
Member

@ShubhamOulkar ShubhamOulkar Jul 15, 2025

Choose a reason for hiding this comment

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

Suggested change
scroll-margin-top: 120px;
scroll-margin-top: 60px;

see comment https://github.com/expressjs/expressjs.com/pull/1965/files#r2206464995

}
Copy link
Member

@bjohansebas bjohansebas Jul 15, 2025

Choose a reason for hiding this comment

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

I'm not sure if this is actually doing anything, and if it's supposed to have the same effect as in the MDN example https://developer.mozilla.org/en-US/docs/Web/CSS/scroll-margin-top, then I'm -1 on this, I don't think we should do it.

Copy link
Member

@ShubhamOulkar ShubhamOulkar Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

without scroll top margin with scroll top margin
image image

We can clearly see that margin spaces added to the header on scroll from the top of the viewport.
This property is used to increase visible area around the target element so that it is accessible to the user.

In our context, why it is important?
I expressed my concerns over the issue of hiding headers behind the top navigation bar in a comment above.

But it is fine if headings are not hiding behind the header bar on anchor link click.

This happens if top navigation is positioned sticky at the top. See image, express.raw() hide behind sticky top navigation.
image

It is not required right now, without scroll top margin, header scroll into viewport at top 0px. But I think adding some small margin 60px to the top looks good for me. Also our header is 57px height. Maybe I will go with sticky top navigation bar in #1943.
image

see scroll_padding_and_margin


/* Respect user motion preferences for access */
@media (prefers-reduced-motion: reduce) {
html {
scroll-behavior: auto;
}
}

body {
Expand Down Expand Up @@ -264,11 +278,6 @@ a {
padding-top: 40px;
}

#api-doc *:target, #page-doc *:target {
Copy link
Member

@ShubhamOulkar ShubhamOulkar Jul 15, 2025

Choose a reason for hiding this comment

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

I support removing this
since we are using scroll top margin only on headers.

margin-top: -120px;
padding-top: 120px;
z-index: -1;
}

/* logo */
.logo-container a {
Expand Down