Skip to content

Conversation

@jplhomer
Copy link
Contributor

Description

Fixes #1141

This adds an additional state to only fire the scroll restoration logic when a location has changed, not when pending has changed.


Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)
  • Update docs in this repository according to your change
  • Run yarn changeset add if this PR cause a version bump based on Keep a Changelog and adheres to Semantic Versioning

@jplhomer jplhomer requested a review from a team April 29, 2022 15:55
@nattyg93
Copy link

nattyg93 commented May 1, 2022

Will this trigger the "scroll to top" behaviour when the pathname does not change, but search does? I ask, because in our app, we often use the search to keep track of filters (form inputs) on a page, and it would break the UX if it scrolled to the top whenever a user updates one of the filters. However, on a different page, we use the search for pagination - at which point we do want it to scroll to top. However, we are able to handle this later case in our code with a useEffect and window.scrollTo(0, 0). So my thinking is that for our particular usecase(s), we either need some sort of escape hatch to say: "please don't scroll the top when I change these only the search" OR don't scroll to the top when the only the search changes and we can handle manually scrolling to the top when our intended UX calls for it.

@jplhomer
Copy link
Contributor Author

jplhomer commented May 2, 2022

Will this trigger the "scroll to top" behaviour when the pathname does not change, but search does?

Correct. This is the behavior you get natively in the browser, and we would want to preserve that.

please don't scroll the top when I change these

Maybe something like Next.js shallow routing is the answer here? Where we'd allow some sort of prop like <Link scrollToTop={false}> which indicates that the URL should change and RSC is fetched, but the scroll position should not change?

@jplhomer
Copy link
Contributor Author

jplhomer commented May 2, 2022

Update: I realized that you can actually accomplish this without us needing a new API:

  1. Manually call setServerProps and use an alternate path, like _search or _filter
  2. Call window.history.pushState() in your client component to update the URL. This does not affect scroll position.
  3. In your server component, look for both params (real search and alternate path). This allows you to present the correct filter values for both initial page loads and realtime RSC updates.

@jplhomer jplhomer merged commit d3e3e69 into v1.x-2022-07 May 2, 2022
@jplhomer jplhomer deleted the jl-fix-scroll-restore branch May 2, 2022 21:13
blittle added a commit that referenced this pull request May 4, 2022
* v1.x-2022-07: (95 commits)
  [ci] release v1.x-2022-07 (#1170)
  Try ignoring hello-world to see if it will get bumped
  Don't consider examples part of the workspace (#1202)
  Fix headers on oxygen (#1201)
  Add bot user agents for Seoradar and Adresults, resolves #1199 (#1200)
  Fix changeset
  updates to docker deploy documentation to resolve run issues (#1196)
  Upgrade body-parser (#1162)
  Fix path for deployments
  Adds ability to add more than one cookie per response (#1161)
  Move Demo Store to templates folder (#1132)
  Avoid additional div element (#1191)
  Whoops this should only be patch
  Adds preconnect <link> for CDN (#1160)
  Bump ejs from 3.1.6 to 3.1.7 (#1147)
  Fix scroll restoration when server props are changed (#1152)
  Typo
  Fixes #1165 by making a missing alt tag a console warning (#1167)
  Remove concurrency directive for Oxygen deployments
  Fix hydrogen-ui dev and build issues (#1169)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] The return of "Router scrolls to top whenever server state is finished being updated"

3 participants