-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
[1.x] Fix a flash of scrolling-to-top on page changes by using useLayoutEffect
#1803
Conversation
@@ -484,7 +496,7 @@ export class Router { | |||
Promise.resolve(this.resolveComponent(page.component)).then((component) => { | |||
if (visitId === this.visitId) { | |||
this.page = page | |||
this.swapComponent({ component, page, preserveState: false }).then(() => { | |||
this.swapComponent({ component, page, preserveState: false, preserveScroll: true }).then(() => { | |||
this.restoreScrollPositions() |
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.
As far as I can tell, restoreScrollPositions()
doesn't suffer from this same issue in my testing. I'm not entirely sure why, to be honest.
setCurrent((current) => ({ | ||
component, | ||
page, | ||
key: preserveState ? current.key : Date.now(), | ||
preserveScroll: !!preserveScroll, |
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.
both preserveState
and preserveScroll
are typed as a PreserveStateOption
in the router core, although they actually seem to be resolved to booleans where it matters in the usage here?
This is where my knowledge of this codebase starts to feel super minimal -- so I may be way off here
@@ -5,7 +5,7 @@ export default function Layout({ children }) { | |||
|
|||
return ( | |||
<> | |||
<nav className="flex items-center space-x-6 bg-slate-800 px-10 py-6 text-white"> | |||
<nav className="sticky top-0 flex items-center space-x-6 bg-slate-800 px-10 py-6 text-white"> |
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.
allows for reproduction to be tested -- you need to be able to click a link while in a scrolled-down state
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.
Just tested this locally and reproduced it, adding a red background to the article title and throttling CPU and network makes it easier to spot it. Nice work @oscarnewman 👏
Before fix
before-fix.mov
After fix
after-fix.mov
The core has been rewritten to support async requests, the code that handles scroll moved to the packages/core/src/scroll.ts
file. Let's wait until that work is merged into master
to rebase and merge this one 👍
#1796 also fixes a scroll restoration bug in the React adapter. These PRs conflict with each other but that will be easy to consolidate.
@@ -40,19 +40,23 @@ export class Router { | |||
protected navigationType?: string | |||
protected activeVisit?: ActiveVisit | |||
protected visitId: VisitId = null | |||
protected handleScrollResetExternally: boolean = false |
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.
Suggestion: customScrollResetHandler
@@ -134,6 +138,14 @@ export class Router { | |||
} | |||
} | |||
|
|||
protected resetScrollPositionsIfNotHandledExternally(): void { |
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.
Suggestion: attemptScrollReset
While searching for ways simplify this solution and trying to avoid handling scroll outside I tested it and it solves the scrolling glitch in React. Since this pattern is safe for the other adapters, we can implement a fix inside We can also replace it where we currently use a |
useLayoutEffect
useLayoutEffect
This is my (attempted) fix for #1802. I outlined the issue in detail there but tl;dr:
<Link>
.A huge caveat to all of this is that I'm brand new to Inertia as a user, let alone looking at this codebase, so please take anything I suggest with a grain of salt.
Based on how Remix/React-router handle this successfully with a
useLayoutEffect
, I've slightly adapted a few things:router.init
whether it will handle resetting scroll position itself (vs letting the router core decide when to do it)swapComponent
callback withinrouter.init
now passes a new argument,preserveScroll
, similar to how it passespreserveState
. This and (1) mean React has the information needed to manage scroll resetting in it's own render lifecyclerouter.resetScrollPositions()
is now publicrouter.resetScrollPositions()
adjacent to theswapComponent
call have been replaced withrouter.resetScrollPositionsIfNotHandledExternally()
-- which just does nothing if thehandleScrollResetsExternally
flag is set by the framework package. Other calls torouter.resetScrollPositions()
weren't changed as they don't interact with theswapComponent
function and don't seem to suffer the same issueI've also updated the React playground with a
sticky
header so that the fix can be validated there as well.Testing so far:
preserveScroll
still works onLink
andRouter
calls ✅