Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix scroll restoration #3191

Merged
merged 32 commits into from
Jan 29, 2025
Merged

fix: Fix scroll restoration #3191

merged 32 commits into from
Jan 29, 2025

Conversation

schiller-manuel
Copy link
Contributor

No description provided.

Copy link

nx-cloud bot commented Jan 18, 2025

View your CI Pipeline Execution ↗ for commit a40e193.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 3m 27s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 22s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-29 21:49:52 UTC

Copy link

pkg-pr-new bot commented Jan 18, 2025

Open in Stackblitz

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/@tanstack/arktype-adapter@3191

@tanstack/create-router

npm i https://pkg.pr.new/@tanstack/create-router@3191

@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/@tanstack/directive-functions-plugin@3191

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/@tanstack/eslint-plugin-router@3191

@tanstack/history

npm i https://pkg.pr.new/@tanstack/history@3191

@tanstack/react-cross-context

npm i https://pkg.pr.new/@tanstack/react-cross-context@3191

@tanstack/create-start

npm i https://pkg.pr.new/@tanstack/create-start@3191

@tanstack/react-router

npm i https://pkg.pr.new/@tanstack/react-router@3191

@tanstack/react-router-with-query

npm i https://pkg.pr.new/@tanstack/react-router-with-query@3191

@tanstack/router-cli

npm i https://pkg.pr.new/@tanstack/router-cli@3191

@tanstack/router-core

npm i https://pkg.pr.new/@tanstack/router-core@3191

@tanstack/router-devtools

npm i https://pkg.pr.new/@tanstack/router-devtools@3191

@tanstack/router-generator

npm i https://pkg.pr.new/@tanstack/router-generator@3191

@tanstack/router-plugin

npm i https://pkg.pr.new/@tanstack/router-plugin@3191

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/@tanstack/router-vite-plugin@3191

@tanstack/server-functions-plugin

npm i https://pkg.pr.new/@tanstack/server-functions-plugin@3191

@tanstack/start

npm i https://pkg.pr.new/@tanstack/start@3191

@tanstack/start-api-routes

npm i https://pkg.pr.new/@tanstack/start-api-routes@3191

@tanstack/start-client

npm i https://pkg.pr.new/@tanstack/start-client@3191

@tanstack/start-config

npm i https://pkg.pr.new/@tanstack/start-config@3191

@tanstack/start-plugin

npm i https://pkg.pr.new/@tanstack/start-plugin@3191

@tanstack/start-router-manifest

npm i https://pkg.pr.new/@tanstack/start-router-manifest@3191

@tanstack/start-server

npm i https://pkg.pr.new/@tanstack/start-server@3191

@tanstack/start-server-functions-client

npm i https://pkg.pr.new/@tanstack/start-server-functions-client@3191

@tanstack/start-server-functions-fetcher

npm i https://pkg.pr.new/@tanstack/start-server-functions-fetcher@3191

@tanstack/start-server-functions-handler

npm i https://pkg.pr.new/@tanstack/start-server-functions-handler@3191

@tanstack/start-server-functions-server

npm i https://pkg.pr.new/@tanstack/start-server-functions-server@3191

@tanstack/start-server-functions-ssr

npm i https://pkg.pr.new/@tanstack/start-server-functions-ssr@3191

@tanstack/valibot-adapter

npm i https://pkg.pr.new/@tanstack/valibot-adapter@3191

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/@tanstack/virtual-file-routes@3191

@tanstack/zod-adapter

npm i https://pkg.pr.new/@tanstack/zod-adapter@3191

commit: a40e193

@schiller-manuel schiller-manuel force-pushed the fix-scroll-restoration branch 2 times, most recently from 67f86d1 to cb748b9 Compare January 18, 2025 11:00
@tannerlinsley tannerlinsley force-pushed the fix-scroll-restoration branch from 0330b37 to bda0550 Compare January 28, 2025 00:07
tannerlinsley and others added 12 commits January 28, 2025 02:04
… in lazyRouteComponent (#3262)

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… during SSR (#3268)
RELEASE_ALL
tannerlinsley and others added 9 commits January 29, 2025 09:05
…router into fix-scroll-restoration

# Conflicts:
#	packages/react-router/src/Match.tsx
#	packages/react-router/src/router.ts
…router into fix-scroll-restoration
@tannerlinsley tannerlinsley merged commit fdaefc0 into main Jan 29, 2025
5 checks passed
@tannerlinsley tannerlinsley deleted the fix-scroll-restoration branch January 29, 2025 21:50
@rolftimmermans
Copy link
Contributor

I found this change broke native browser scroll restoration in our app. This is because setupScrollRestoration() is called even when scrollRestoration is set to false. Is this intentional, or could there be a way to opt out entirely? I was happy with browser-native scroll restoration.

@tannerlinsley
Copy link
Collaborator

It’s called because it also handles hash scrolling. The actual restoration logic internally only runs if it’s turned on.

@rolftimmermans
Copy link
Contributor

rolftimmermans commented Feb 11, 2025

It’s called because it also handles hash scrolling. The actual restoration logic internally only runs if it’s turned on.

Thanks for your response! Much appreciated.

I guess the issue is: if no restoration happens, the page is scrolled to the top. See:

This counteracts native scroll restoration. And it seems to be different behaviour than before this PR. Ideally I'd like a way to opt out of that altogether. Also OK if it means opting out of hash scrolling.

I'd be happy to create a PR for it. But of course only if you agree this is a valid option to provide. And what exactly the option should do – opt out of hash scrolling as well? or only opt out of the default scrolling to the top in case there is nothing to do?

@mihkelmark
Copy link

This release also broke the scroll restoration in my app.

After 30 minutes of reading the docs and trying to figure out how to have my app (with simple use cases) support the new version, I haven't succeeded. So I'm either thick or the documentation is not clear about this. Which is unlike the tanstack suite, so I'm guessing it's me.

I'm reverting back to the old version for now. Reluctantly, but will have to do it to move forward.

@tannerlinsley
Copy link
Collaborator

Would you have time to jump on a call soon to debug with me?

@tannerlinsley
Copy link
Collaborator

Dm me in discord.

@rolftimmermans
Copy link
Contributor

rolftimmermans commented Mar 12, 2025

For @mihkelmark or others in this thread; the reason things worked with native scroll restoration in the past was more or less accidental. Architectural changes in @tanstack/react-router after 1.98 require scroll restoration to be configured. In my case this was a matter of adding:

const router = createRouter({
  scrollRestoration: true,
  // ...
})

And in the virtualized tables in our application:

const restoration = useElementScrollRestoration({getElement: () => window})
const virtualizer = useWindowVirtualizer({
  initialOffset: restoration?.scrollY ?? 0,
  // ...
})

That is all!

Returning window from getElement() was not possible before yesterday, but @tannerlinsley graciously fixed this when investigating this use case with useWindowVirtualizer().

@schiller-manuel
Copy link
Contributor Author

schiller-manuel commented Mar 12, 2025

@rolftimmermans we should add the window part to the docs !
want to provide a PR to the docs? with a small example?

@rolftimmermans
Copy link
Contributor

@rolftimmermans we should add the window part to the docs ! want to provide a PR to the docs? with a small example?

Sure! #3752

schiller-manuel pushed a commit that referenced this pull request Mar 12, 2025
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.

None yet

7 participants