-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Allow scrolling of individual elements #10468
base: dev
Are you sure you want to change the base?
Conversation
See remix-run#9495 and remix-run#9573; based on work there.
🦋 Changeset detectedLatest commit: b1bbd43 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Hi @joshkel, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
Nice. Would love to see this merged, but in the meantime I'll test it with patch-package. |
Uh, yeah, I managed to stumble across the |
@joshkel Thanks for the PR (and thanks @david-crespo for the initial PR this is based on)! I did some local testing and found/fixed one small issue if an app has different I also added support for passing a CSS selector instead of a ref and thus changed the prop name to Let me know if this still looks good to you and I think we should be able to get this merged! |
@@ -81,6 +104,32 @@ Or you may want to only use the pathname for some paths, and use the normal beha | |||
/> | |||
``` | |||
|
|||
## `scrollContainer` | |||
|
|||
By default, this component will capture/restore scroll positions on `window`. If your UI scrolls via a different container element with `overflow:scroll`, you will want to use that instead of `window`. You can specify that via the `scrollContainer` prop using a selector or a `ref`: |
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 don't know if there's simple guidance about whether to prefer ref or selector that fits in a sentence, but if there is, it would be good to add it. I feel ref
is safer and easier to be confident about. But maybe people can figure that out.
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.
Yeah, hard to say. I sort of envisioned refs being used more when the scrollable thing is easily accessible from the component rendering <ScrollRestoration>
and selectors if it's a few components away and you don't want to prop-drill a ref around. But I'm not sure if one is technically any better than another?
Thanks for the PR! I've wanted to allow for this for a long time but I think it needs a bit more design than what's here. The main thing is you should be able to have multiple scroll containers at the same time, not just one (like both Additionally, this should be able to be done outside of react router with |
@ryanflorence, I think being able to support multiple scroll containers here would mostly be matter of the shape of the savedScrollPosition object, perhaps a map of keys containing another map of scrollContainer identifiers/css selectors, etc. (of some kind) to their respective scrollPositions. Although that gets a bit trickier when you use refs rather than selectors for the containers though, since you've got to serialize and deserialize from sessionStorage (at least with this current approach).
Can you elaborate a bit more on what you're thinking here? |
I'm interested in trying to write this hook from userland. Seems like the main reason it's integrated into RR deeply is so it can detect when a nav is about to happen, so it can save the scroll position. With |
I think this might be where useBlocker comes in handy. |
Oops, I meant |
But useNavigation doesn't do anything useful unless you're doing async loaders, right? EDIT: what I mean here is that there's not any useful info in the navigation object unless you're using async loaders, etc., right? |
This seems to work. Lol. Lmao. export function useScrollRestoration(elementRef: React.RefObject<HTMLElement>) {
const cache = useRef(new Map<string, number>())
const { key } = useLocation()
const { state } = useNavigation()
useEffect(() => {
if (state === 'loading') {
cache.current.set(key, elementRef.current?.scrollTop ?? 0)
} else if (state === 'idle' && cache.current.has(key)) {
elementRef.current?.scrollTo(0, cache.current.get(key)!)
}
}, [key, state, elementRef])
} |
oh wow, @david-crespo , I'm gonna give that a try! |
@ryanflorence, am I correct in saying that you'd be fine with (or even prefer) a solution that isn't coupled to the existing ScrollRestoration functionality? |
I mean, if my thing is all it takes, I'd argue it doesn't even need to be built in. An example somewhere in the docs might make more sense. And yeah, |
Updated to use import { useEffect } from 'react'
import { useLocation, useNavigation } from 'react-router-dom'
function getScrollPosition(key: string) {
const pos = window.sessionStorage.getItem(key)
return pos && /^[0-9]+$/.test(pos) ? parseInt(pos, 10) : 0
}
function setScrollPosition(key: string, pos: number) {
window.sessionStorage.setItem(key, pos.toString())
}
/**
* Given a ref to a scrolling container element, keep track of its scroll
* position before navigation and restore it on return (e.g., back/forward nav).
* Note that `location.key` is used in the cache key, not `location.pathname`,
* so the same path navigated to at different points in the history stack will
* not share the same scroll position.
*/
export function useScrollRestoration(container: React.RefObject<HTMLElement>) {
const key = `scroll-position-${useLocation().key}`
const { state } = useNavigation()
useEffect(() => {
if (state === 'loading') {
setScrollPosition(key, container.current?.scrollTop ?? 0)
} else if (state === 'idle') {
container.current?.scrollTo(0, getScrollPosition(key))
}
}, [key, state, container])
} |
@david-crespo , correct me if I'm wrong, but if you're not using async loaders, the scroll positions would never get set, right? I don't use them yet, and navigation.state is always |
Yeah, I wasn't sure how that would work. I thought it was possible that a data router would always do async page transitions even if loaders weren't async. When you say async loaders, you mean you do have loaders but they're synchronous? Or you're not using loaders at all? In either case, you could probably fix that by adding a root loader that's just |
@david-crespo , I meant we're not using loaders at all yet. |
@david-crespo, looks like a pagehide handler is still going to be needed to capture the scroll position for page reloads. But otherwise, the general approach seems to work. |
@ryanflorence - @david-crespo is correct in that right now the reason this goes deeper than The other part we currently handle through the @jrnail23 I believe as long as you have loaders, even if they're synchronous, the |
@brophdawg11 yep, I've added one dummy loader at the root as @david-crespo suggested, and it's working as expected ( |
Definitely a stupid idea, but maybe the simplest fix is to have the router go into the loading state whether you have loaders or not — essentially building in the dummy loader. But that would mean this only works for data router. |
I had the same thought, @david-crespo, so it must be a terrible idea 😛 . |
Hi, I have encountered the same problem about ScrollRestoration on a Table. |
Not much going on here but if you're using data router, I had success implementing it in userland — I actually prefer it over the built in version because it's so simple. |
If anyone is stumbling across a similar issue, I have been using #10468 (comment) by @david-crespo in production but had occasional bugs where the scroll restoration did not work. Turns out in some cases import { useEffect } from 'react'
import { useLocation, useNavigation } from 'react-router-dom'
function getScrollPosition(key: string) {
const pos = window.sessionStorage.getItem(key)
return Number(pos) || 0;
}
function setScrollPosition(key: string, pos: number) {
window.sessionStorage.setItem(key, pos.toString())
}
/**
* Given a ref to a scrolling container element, keep track of its scroll
* position before navigation and restore it on return (e.g., back/forward nav).
* Note that `location.key` is used in the cache key, not `location.pathname`,
* so the same path navigated to at different points in the history stack will
* not share the same scroll position.
*/
export function useScrollRestoration(container: React.RefObject<HTMLElement>) {
const key = `scroll-position-${useLocation().key}`
const { state } = useNavigation()
useEffect(() => {
if (state === 'loading') {
setScrollPosition(key, container.current?.scrollTop ?? 0)
} else if (state === 'idle') {
container.current?.scrollTo(0, getScrollPosition(key))
}
}, [key, state, container])
} |
See #9495.
This is an updated version of #9573, in hopes of moving things along.
This version uses refs, since the linked discussion seemed to prefer that interface.