Do not scroll to top if the URL pathname has not changed #813
Conversation
|
Comparing this branch to main it's an improvement, but I'm still seeing some unnecessary (and probably unrelated) scroll jumps. Screen.Recording.2022-03-04.at.15.13.44.mov |
|
@cartogram Apparently, the mobile menu was triggering a different reset by default. |
This reverts commit c40d022.
…ller chunks (#832) * Change RouterClient to be a shared component to reduce server bundle size * Change ServerStateProvider to be a shared component and avoid extra bundling in client build * Update changeset
| window.scrollTo(0, 0); | ||
| } | ||
|
|
||
| currentPath = serverState.pathname; | ||
| }, [pending]); |
There was a problem hiding this comment.
Should the effect dependency include serverState.pathname?
There was a problem hiding this comment.
As far as I understand, pending already changes every time the server state is updated since the whole thing is a React transition. Do we still need to add serverState.pathname in that case? 🤔
@jplhomer I think it's fine. A more "react" way would be to use |
|
I might add a e2e test for this. I know I fixed this back in October when I first joined. We've since regressed again with the same issue. :( |
|
About mutating the module variable, this should only happen in the browser and we only support 1 instance of these components. The problem with having these variables within React is that mutations were triggering extra refreshes and breaking hydration. We'd need to wrap it all in |
Description
Closes #810
The scroll should only be reset when the pathname changes. Or is there any use case where this is different?
Before submitting the PR, please make sure you do the following:
fixes #123)yarn changeset addif this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning