-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
refactor: cleanup scroll handlers #5709
Conversation
} | ||
}, | ||
[location], |
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.
To check location we now use useLocationChange
hook everywhere, this will better distinguish between scroll and location event handlers.
|
||
useEffect(() => { | ||
// Prevent first effect to trigger the listener on mount |
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.
This is unnecessary code, it used to be needed to fix #5020, but it is safe to remove now.
✔️ [V2] 🔨 Explore the source changes: 8065b89 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61694f6c2128540008eb7c2a 😎 Browse the preview: https://deploy-preview-5709--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5709--docusaurus-2.netlify.app/ |
Size Change: -46 B (0%) Total Size: 841 kB
ℹ️ View Unchanged
|
Didn't notice any issue 👍 |
Motivation
This PR tries to unify use of scroll handlers in
useScrollPosition
, as there are quite a few non-obvious and unnecessary things right now. Beside that, it improves performance a bit, at least hideable navbar is no longer rendered twice on first mount.It also fixes proper closing of hideable navbar when navigating from search (eg, "docs/next/versioning#docs"):
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Preview. It's not that simple, you have to check hideable navbar and back-to-top-button.
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)