search

epicweb-dev / restore-scroll

🌀 Restore scroll position of elements on page navigation
https://github.com/epicweb-dev/restore-scroll
249 stars 5 forks source link

Requires a loader #9

Open OliverJAsh opened 2 months ago

OliverJAsh commented 2 months ago

If I understand correctly, this will update positions when the navigation state is not idle, e.g. when it's loading. This will only happen if the next route has a loader.

Here's a reduced test case to illustrate the problem: https://stackblitz.com/edit/github-eve2dr-vm34va?file=src%2Fapp.tsx

What are your thoughts on updating this to save the scroll position even when navigating to a route that doesn't have a loader?

OliverJAsh commented 2 months ago

On a related note, I also noticed that it doesn't work when v7_startTransition is enabled. This is because useNavigation().state is always idle.

https://stackblitz.com/edit/github-eve2dr-glrg3x?file=src%2Fapp.tsx

kentcdodds commented 2 months ago

@brophdawg11, do you think React Router could publish an official version of this so we don't have to try to integrate from the outside? It's proving to be a challenge.

brophdawg11 commented 2 months ago

I'll check with Ryan and Michael on their appetite for inlining! I do think that a foolproof solution to this likely requires access to router internals for this specific no-loader case, and also for more advanced preventScrollReset behaviors - see https://github.com/remix-run/react-router/pull/10468#issuecomment-1578935980.

That community PR was only for changing the scroll container, not supporting multiple scroll containers - so maybe this package addresses the shortcoming that stalled out that PR.

kentcdodds commented 2 months ago

Thanks @brophdawg11!

In the mean time @OliverJAsh, I'm afraid I only have the bandwidth to address issues that are impacting me and my own work at the time. I'm happy to look at a PR though.