next-page-transitions
next-page-transitions copied to clipboard
fix: monkeyPatchScrolling to scroll back to top during transitions
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.
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.
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.
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 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.