-
-
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
fix(v2): ignore hash changes in useChangeRoute hook #5023
Conversation
✔️ [V2] 🔨 Explore the source changes: e1d7e0f 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60d1bb8adf0c4d0008fa645a 😎 Browse the preview: https://deploy-preview-5023--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5023--docusaurus-2.netlify.app/ |
Size Change: +133 B (0%) Total Size: 569 kB
ℹ️ View Unchanged
|
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.
I'd rather make this fix outside of the onRouteChange hook
const latestPathnameRef = useRef(pathname); | ||
|
||
useEffect(() => { | ||
if (pathname !== latestPathnameRef.current) { | ||
if (!hash && pathname !== latestPathnameRef.current) { | ||
latestPathnameRef.current = pathname; | ||
onRouteChange(); |
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.
What about using onRouteChange({location})
instead, and preventing the call to programmaticFocus
if there's a hash?
onRouteChange
should rather fire even if an anchor link is clicked, for example this should be triggered, otherwise the hook does not really do what its name implies.
We also may want this callback to keep being called for anchor links:
useChangeRoute(() => {
if (!hideOnScroll) {
return;
}
setIsNavbarVisible(true);
});
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.
I thought about this at first, but for the hideable navbar to work properly we also need to ignore hash changes in the according hook, otherwise the navbar will automatically show up when go to on anchor link on cross-page. So in order not to duplicate code, we can slightly improve useChangedRoute
hook directly, because at the moment it also triggers when hash has changed, but it shouldn't. After all, this hook is only supposed to keep track of changes in current pathname, right?
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.
Yes but it won't trigger the callback when current pathname change, if a hash is also present, which is confusing.
Agree to merge that for now, but not 100% sure it'll be the best long-term decision. It might become a problem if we decide to use this hook for another use-case. For example it wouldn't work reliably anymore for handling analytics or things like that, because it wouldn't report page changes if user keeps clicking on anchor links.
If we use more this hook in the future we'll have to handle the hash case on the 2 call-sites we currently have
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.
Actually I'll do that now, because I'm not comfortable at all having the latestPathnameRef
not being updated when there is a hash. This might lead to subtle bugs in this hook after the user has navigated to an anchor link
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.
Alright, although there is more code/complexity, agree, it looks more flexible and reliable 👍
Hey, any idea on when this is going to be released? We really want to use the new |
@tsirlucas you can always use the canary release: https://docusaurus.io/community/canary Going to release very soon! |
@slorber Thank you, where will be possible to see the release for this fix? |
@indre-idenfy this is already deployed in the beta.2 but unfortunately theres a new bug introduced so im waiting for beta.3 |
Motivation
Fixes #5020
Currently the
useChangedRoute
hook is mistakenly triggered by hash changes on cross-page, which in particular resets scroll position when clicking on anchor link leading to another internal page.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
See "Reproducible Demo" section from #5020.
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.)