-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: fix issues related to page scrolling #6641
Conversation
@nodejs/website @nodejs/documentation Is the JS "hack" too much? While I think it's pretty solid, I think that at this point I'm ok with removing it if someone has concerns about it. |
Just general ones. Haven't made it to review yet. If you can avoid it it would be better.... |
Since it is using vanilla and well supported JS, I'd be okay with having this now and hopefully removing this later. |
this.scrollTop = e.deltaY > 0 ? this.scrollHeight : 0; | ||
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.
If you go this route, may I suggest to throttle the listener to avoid horrible perfs janks?
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.
The handler is pretty lightweight, I don't think it could cause any janks, except on maybe ancient hardware.
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.
Isn't it still good practice to throttle it regardless?
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.
Depends on the use case. With throttling, some events won't get cancelled which results in scroll ticks going through. That's not something I want in this case, so every event would have to be cancelled.
Hmm, I think I'll drop the JS after all. It was just one user complaining about it, while probably more people agree to let the browser handle scrolling. |
Moved the sidebar to a fixed position and moved the positioning of the main content to the page's body, which results in back/forward navigation through hash links and search highlight working again. Fixes: nodejs#6637 Based on: nodejs#5716
Okay, CSS only change now to fix two issues. Please review! |
I've tested this on Chrome and mobile Safari, all good 👍 . |
LGTM then |
Moved the sidebar to a fixed position and moved the main column to the page's body, which results in back/forward navigation through hash links and search highlight working again. Fixes: #6637 Fixes: #6751 Based on: #5716 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com>
Landed in bef1ec0. |
Checklist
Description of change
Moved the sidebar to a fixed position and moved the positioning of the main content to the page's body, which results in back/forward navigation through hash links and search highlight working again.
Added a script to prevent the main column from unintentional scrolling when the sidebar is scrolled outside its boundaries.
Fixes: #6637
Based on: #5716