-
-
Notifications
You must be signed in to change notification settings - Fork 872
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(react-router): fix issue with scroll restoration #3918
Conversation
…oration component behind the flag
packages/react-router/src/Match.tsx
Outdated
@@ -113,10 +113,10 @@ export const Match = React.memo(function MatchImpl({ | |||
</ResolvedCatchBoundary> | |||
</ResolvedSuspenseBoundary> | |||
</matchContext.Provider> | |||
{parentRouteId === rootRouteId && router.options.scrollRestoration ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schiller-manuel It's just a bit strange that if I don't enable scrollRestoration, I won't be able to subscribe to onRendered router events. From my perspective, those shouldn't be connected. I'm not sure what the best solution here is. Maybe it can be just useEffect since we don't need to have access to DOM. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you need this event for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schiller-manuel I don't use it in my projects, but if it's listed in API documentation, people might use it to run some side effects. It is not obvious that it only works if scrollRestoration is enabled. Also in the Solid version of the router it is not connected to the scrollRestoration feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's just meant as an internal event really. so for now please dont apply this change until we clarify this.
View your CI Pipeline Execution ↗ for commit 4fa1f74.
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/create-router
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/create-start
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-with-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-config
@tanstack/react-start-plugin
@tanstack/react-start-router-manifest
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-config
@tanstack/solid-start-plugin
@tanstack/solid-start-router-manifest
@tanstack/solid-start-server
@tanstack/start
@tanstack/start-api-routes
@tanstack/start-client-core
@tanstack/start-config
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-handler
@tanstack/start-server-functions-server
@tanstack/start-server-functions-ssr
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
fixes #3680
It seems like
resolvedLocation
does not always point to the latest location state in the ref callback; sometimes it points to the previous location andonRendered
is not triggered.