next-page-transitions icon indicating copy to clipboard operation
next-page-transitions copied to clipboard

fix: monkeyPatchScrolling to scroll back to top during transitions

Open ryanhefner opened this issue 5 years ago • 4 comments

Make call to window.scroll onEnter after scrolling has been re-enabled. Fixes issue that primarily affects mobile browsers, where the page position is inconsistent between navigations.

ryanhefner avatar Jul 10 '19 16:07 ryanhefner

It was initially unclear to me what this code was meant to do, although I think I understand now. To clarify: this captures the last call to window.scrollTo made during the transition, specifically the one here: https://github.com/zeit/next.js/blob/71f9288a54c32420a18a07035ea7a73c9c3c50e5/packages/next/client/link.tsx#L178, and then performs it after the new page has started entering?

This definitely needs some documentation - both in the code, to explain why we're doing this, and on the Readme, to make it clear that we're doing this at all. I don't anticipate that this should cause any problems, as people probably aren't calling window.scrollTo during transition anyways. Nonetheless, this behavior should be documented.

nwalters512 avatar Jul 29 '19 05:07 nwalters512

Hey, thanks for getting back with me on this. I just submitted a few commits to address the prettier issue and added some comments to the code. Although, thinking about this more, I am wondering about what the expected behavior of this package should be. I know that the monkeyPatchScrolling thing was kind of a hack, but at the same time with this commit, it makes the whole package work more how I would expect it to work out of the box.

I know this might sound crazy, but should this be refactored a bit more to just make this the default behavior?

Alternatively, I could also see a world where the where the different handlers (onEnter, onEntering, onEntered, onExit, onExiting, etc.) were exposed, and if you were to assume that monkeyPatchScrolling would just disable scrolling altogether, exposing those callbacks so that people could handle the scrolling themselves during the lifecycle could provide flexibility to people working with this package to easily handle scrolling as they please.

Sorry, kind of went off on a few tangents there, but would love to know your thoughts.

And, for some context, at this point I have used this package on 3 or so projects, and we have enabled monkeyPatchScrolling within all of them, to avoid jumpy/flickery effect that you get when you don’t enable it.

ryanhefner avatar Aug 01 '19 15:08 ryanhefner

Thanks for this PR. It fixed an issue similar to https://github.com/illinois/next-page-transitions/issues/33 and https://github.com/illinois/next-page-transitions/issues/41 where on Safari mobile and iPad it would not scroll during the transition. This PR fixes that for me - cheers 👍

chasevida avatar Jul 13 '20 04:07 chasevida

@chasevida Glad this worked for you! I will go back and address the asks from @nwalters512 and see if we can get this into the master.

Let me know if you run Into any other issues that might need to be addressed.

ryanhefner avatar Jul 13 '20 15:07 ryanhefner