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

[1.x] Fix a flash of scrolling-to-top on page changes by using useLayoutEffect #1803

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions packages/core/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,19 +40,23 @@ export class Router {
protected navigationType?: string
protected activeVisit?: ActiveVisit
protected visitId: VisitId = null
protected handleScrollResetExternally: boolean = false
Copy link
Collaborator

@pedroborges pedroborges Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: customScrollResetHandler


public init({
initialPage,
resolveComponent,
swapComponent,
handleScrollResetExternally
}: {
initialPage: Page
resolveComponent: PageResolver
swapComponent: PageHandler
handleScrollResetExternally?: boolean
}): void {
this.page = initialPage
this.resolveComponent = resolveComponent
this.swapComponent = swapComponent
this.handleScrollResetExternally = !!handleScrollResetExternally

this.setNavigationType()
this.clearRememberedStateOnReload()
Expand Down Expand Up @@ -116,7 +120,7 @@ export class Router {
})
}

protected resetScrollPositions(): void {
public resetScrollPositions(): void {
window.scrollTo(0, 0)
this.scrollRegions().forEach((region) => {
if (typeof region.scrollTo === 'function') {
Expand All @@ -134,6 +138,14 @@ export class Router {
}
}

protected resetScrollPositionsIfNotHandledExternally(): void {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: attemptScrollReset

if (this.handleScrollResetExternally) {
return
}

this.resetScrollPositions()
}

protected restoreScrollPositions(): void {
if (this.page.scrollRegions) {
this.scrollRegions().forEach((region: Element, index: number) => {
Expand Down Expand Up @@ -455,9 +467,9 @@ export class Router {
page.rememberedState = page.rememberedState || {}
replace = replace || hrefToUrl(page.url).href === window.location.href
replace ? this.replaceState(page) : this.pushState(page)
this.swapComponent({ component, page, preserveState }).then(() => {
this.swapComponent({ component, page, preserveState, preserveScroll }).then(() => {
if (!preserveScroll) {
this.resetScrollPositions()
this.resetScrollPositionsIfNotHandledExternally()
}
if (!replace) {
fireNavigateEvent(page)
Expand All @@ -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()
Copy link
Author

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.

fireNavigateEvent(page)
})
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,12 @@ export type PageHandler = ({
component,
page,
preserveState,
preserveScroll,
}: {
component: Component
page: Page
preserveState: PreserveStateOption
preserveScroll: PreserveStateOption
}) => Promise<unknown>

export type PreserveStateOption = boolean | string | ((page: Page) => boolean)
Expand Down
13 changes: 11 additions & 2 deletions packages/react/src/App.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createHeadManager, router } from '@inertiajs/core'
import { createElement, useEffect, useMemo, useState } from 'react'
import { createElement, useEffect, useLayoutEffect, useMemo, useState } from 'react'
import HeadContext from './HeadContext'
import PageContext from './PageContext'

Expand All @@ -15,6 +15,7 @@ export default function App({
component: initialComponent || null,
page: initialPage,
key: null,
preserveScroll: false,
})

const headManager = useMemo(() => {
Expand All @@ -25,17 +26,25 @@ export default function App({
)
}, [])

useLayoutEffect(() => {
if (!current.preserveScroll) {
router.resetScrollPositions()
}
}, [current])

useEffect(() => {
router.init({
initialPage,
resolveComponent,
swapComponent: async ({ component, page, preserveState }) => {
swapComponent: async ({ component, page, preserveState, preserveScroll }) => {
setCurrent((current) => ({
component,
page,
key: preserveState ? current.key : Date.now(),
preserveScroll: !!preserveScroll,
Copy link
Author

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

}))
},
handleScrollResetExternally: true,
})

router.on('navigate', () => headManager.forceUpdate())
Expand Down
2 changes: 1 addition & 1 deletion playgrounds/react/resources/js/Components/Layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Author

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

<div className="rounded-lg bg-slate-700 px-4 py-1">{appName}</div>
<Link href="/" className="hover:underline">
Home
Expand Down
Loading