-
-
Notifications
You must be signed in to change notification settings - Fork 696
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
feat(react-router): implement hash change "Scroll Into View" option #2996
feat(react-router): implement hash change "Scroll Into View" option #2996
Conversation
this looks great. let's wait for @SeanCassiere to review |
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.
LGTM
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'm not at home at the moment, so I'll have to check the sandbox locally at home tomorrow.
I'd also like us to think a bit more on the property name. The name hashChangeScrollIntoView
just feels a bit off. Maybe even hashScrollIntoView
?
Approving for now, but I'd like to get a chance to play with the sandbox before merging in.
I believe I've addressed all of the feedback that's come in so far, please let me know if anything else comes up. I added the |
@@ -1838,6 +1846,9 @@ export class Router< | |||
} | |||
} | |||
|
|||
nextHistory.state.__hashScrollIntoViewOptions = | |||
hashScrollIntoView ?? this.options.defaultHashScrollIntoView ?? 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.
Would this (the ?? true
) break any existing apps?
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.
These options are new, so the value will default to true if they aren't provided.
I read on MDN for scrollIntoView that true is the default argument value, so passing true by default should maintain the behavior that exists today. I don't believe this is a breaking change.
I'm not at my computer right now, but I believe I added a test case and Link on the example that use the default behavior, and it seems to function the same as the released version does for me.
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.
Tried out the sandbox and it looks good.
@schiller-manuel OK to merge from my side.
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 72339b6. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
I was investigating the test failure from scroll restoration and found that if browser (not router)-controlled navigation happens, the I could add another nullish coalescing operator to set the value if it's undefined, like so:
This fixes the test and is working for me in the scroll restoration sandbox. @SeanCassiere, does that sound ok to you? |
Worth giving it a try. |
I am working on an API documentation page that has a number of anchor links. The sidebar contains
Link
elements that expand/collapse sections based on if its hash is active. It works like a table of contents. I also update the hash based on the user's scroll position. If they've scrolled past an anchor, it updates the hash. The issue I was hitting is that updating the hash withnavigate
or a number of other methods actually runsscrollIntoView
, which takes over the user's scrolling in these cases.The change I'm proposing is to add an option to
navigate
andLink
that will allow the consumer to choose to opt out of the defaultscrollIntoView
behavior, or even allow them to customize the scroll behavior if they would like to (e.g., for smooth scrolling instead of instant).Discussed in Discord here: https://discord.com/channels/719702312431386674/1316559407919796334
Another relevant discussion: #1155