remix icon indicating copy to clipboard operation
remix copied to clipboard

feat: Add the ability to override scroll restoration without conditionally rendering the `ScrollRestoration` component

Open filipemir opened this issue 4 years ago • 12 comments

Closes: #2810

This PR adds a mechanism for preventing scroll restoration at the level of an individual link or usage of the location object. Here's how the current solution would be used in a link:

<Link to="#" state={{scroll: false}}>click me</Link>

And via the location object (using search params as an example):

const [searchParams, setSearchParams] = useSearchParams();
searchParams.set("a", "b");
setSearchParams(searchParams, { state: { scroll: false } });

This is still a WIP. I'm unsure how to test the change (as far as I can tell this component is still untested) and whether using the location state is the right way to override/stop the scroll restoration. As such, I haven't yet added tests or documentation. Nonetheless, this does fix a frustrating user experience for us (reproduced here) so I'd welcome any feedback or suggestions.

Also, all credit for this should go to @wladiston. I'm simply opening a PR to implement the solution he suggested because I'd like for our app not to rely on a local copy of the ScrollRestoration component's source code

  • [ ] Docs
  • [ ] Tests

filipemir avatar Apr 14 '22 21:04 filipemir

Hi @filipemir,

Welcome, and thank you for contributing to Remix!

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 CLA Signed label will be added to the pull request.

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

remix-cla-bot[bot] avatar Apr 14 '22 21:04 remix-cla-bot[bot]

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

remix-cla-bot[bot] avatar Apr 14 '22 21:04 remix-cla-bot[bot]

@chaance or @ryanflorence is there anything I can do to help get this or an alternative solution to this problem merged? Happy to receive feedback and attempt to implement it but I'd love to not have to maintain my own copy of ScrollRestoration fork indefinitely.

filipemir avatar Apr 28 '22 23:04 filipemir

You don't have to maintain a fork. Just copy+paste the file and add it to your own app/components folder.

We're so used to just npm installing code, that we don't realize we can simply inline the code we want and customize it as needed.

I have my own <LiveReload/> component that fetches /build/__purge__ before refreshing the page. Then in my Express adapter I use that route to purge my cache instead of on every request.

kiliman avatar Apr 28 '22 23:04 kiliman

@kiliman that's what I meant by maintaining a fork. I wasn't speaking about literally forking the remix repo, just that I have to maintain a copy of the LiveReload component in our own repo. The issue it creates is the same whether it's a repo fork or not: any substantive changes to LiveReload in Remix will have to be ported over to our code base as well. It makes every Remix update an opportunity for a regression here. And if the internals of ScrollRestoration ever change substantially it could become a real problem. I will keep doing this indefinitely if there's no progress on this ticket, but I think it would benefit the project to have an official solution to this problem.

filipemir avatar Apr 29 '22 13:04 filipemir

Thanks @filipemir, we have actually been working on some changes to LiveReload internally which is why we haven't merged this just yet. It's on our radar and we'll let you know when anything changes.

chaance avatar Apr 29 '22 14:04 chaance

Looks like I may have confused the issue. This was about <ScrollRestoration/> and I believe @filipemir is also referring to that, but typed <LiveReload/> as I used that as an example.

Anyway, yes I understand that code may change. But we have to be responsible for our own code either way, whether we write it or import it. I don't see the component changing all that much.

I'm not saying that the PR shouldn't be merged. I'm just saying that our priorities don't always match the team's despite what we may desire.

kiliman avatar Apr 29 '22 15:04 kiliman

Thanks, @chaance. It's not a rush. I just wanted to make sure it wasn't lost in noise. I'll check back in some time if I haven't heard back. Thank for all the hard work :)

filipemir avatar Apr 29 '22 17:04 filipemir

And yes, this is for <ScrollRestoration />. No issues with <LiveReload /> on my end

filipemir avatar Apr 29 '22 18:04 filipemir

And yes, this is for <ScrollRestoration />. No issues with <LiveReload /> on my end

filipemir avatar Apr 29 '22 18:04 filipemir

Please release this. I need this because it is so frustrating right now.

DHFW avatar Oct 18 '22 12:10 DHFW

@DHFW my understanding is that the Remix team has a different fix in the works for this, but it requires an update on React router which is still pending. @chaance correct me if I'm wrong.

filipemir avatar Oct 18 '22 13:10 filipemir

@filipemir Thank you so much for the effort 🙏🏼

This have been implemented as of Remix 1.10.0. You can use the new preventScrollReset on <Link /> https://reactrouter.com/en/main/components/link#preventscrollreset

machour avatar Jan 22 '23 20:01 machour