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

Scroll entire page instead of content pane #2450

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion app/components/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ const JumpToButton = () => {

export function Sidebar({ children }: { children: React.ReactNode }) {
return (
<div className="flex flex-col border-r text-sans-md text-default border-secondary">
<div className="fixed z-sideModal flex h-full w-[14.25rem] flex-col border-r text-sans-md text-default border-secondary">
<div className="mx-3 mt-4">
<JumpToButton />
</div>
Expand Down
11 changes: 5 additions & 6 deletions app/components/TopBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,12 @@ export function TopBar({ children }: { children: React.ReactNode }) {
// It's important that this component returns two distinct elements (wrapped in a fragment).
// Each element will occupy one of the top column slots provided by `PageContainer`.
return (
<>
<div className="flex items-center border-b border-r px-3 border-secondary">
<div className="fixed top-0 z-topBar col-span-2 flex h-[var(--navigation-height)] w-full bg-default">
<div className="flex w-[var(--sidebar-width)] items-center border-b border-r px-3 border-secondary">
{cornerPicker}
</div>
{/* Height is governed by PageContainer grid */}
{/* shrink-0 is needed to prevent getting squished by body content */}
<div className="z-topBar border-b bg-default border-secondary">
<div className="z-topBar flex-1 border-b bg-default border-secondary">
{/* shrink-0 is needed to prevent getting squished by body content */}
<div className="mx-3 flex h-[60px] shrink-0 items-center justify-between">
<div className="flex items-center">{otherPickers}</div>
<div className="flex items-center gap-2">
Expand Down Expand Up @@ -65,6 +64,6 @@ export function TopBar({ children }: { children: React.ReactNode }) {
</div>
</div>
</div>
</>
</div>
)
}
17 changes: 8 additions & 9 deletions app/hooks/use-scroll-restoration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,19 @@ function setScrollPosition(key: string, pos: number) {
}

/**
* Given a ref to a scrolling container element, keep track of its scroll
* position before navigation and restore it on return (e.g., back/forward nav).
* Note that `location.key` is used in the cache key, not `location.pathname`,
* so the same path navigated to at different points in the history stack will
* not share the same scroll position.
* Keep track of scroll position before navigation and restore it on return
* (e.g., back/forward nav). Note that `location.key` is used in the cache key,
* not `location.pathname`, so the same path navigated to at different points in
* the history stack will not share the same scroll position.
*/
export function useScrollRestoration(container: React.RefObject<HTMLElement>) {
export function useScrollRestoration() {
const key = `scroll-position-${useLocation().key}`
const { state } = useNavigation()
useEffect(() => {
if (state === 'loading') {
setScrollPosition(key, container.current?.scrollTop ?? 0)
setScrollPosition(key, window.scrollY ?? 0)
} else if (state === 'idle') {
container.current?.scrollTo(0, getScrollPosition(key))
window.scrollTo(0, getScrollPosition(key))
}
}, [key, state, container])
}, [key, state])
}
3 changes: 2 additions & 1 deletion app/layouts/AuthenticatedLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*
* Copyright Oxide Computer Company
*/
import { Outlet } from 'react-router-dom'
import { Outlet, ScrollRestoration } from 'react-router-dom'

import { apiQueryClient, useApiQueryErrorsAllowed, usePrefetchedApiQuery } from '@oxide/api'

Expand Down Expand Up @@ -43,6 +43,7 @@ export function AuthenticatedLayout() {
<>
<QuickActions />
<Outlet />
<ScrollRestoration />
</>
)
}
Expand Down
19 changes: 11 additions & 8 deletions app/layouts/helpers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*
* Copyright Oxide Computer Company
*/
import { useRef } from 'react'
import { Outlet } from 'react-router-dom'

import { PageActionsTarget } from '~/components/PageActions'
Expand All @@ -14,20 +13,21 @@ import { useScrollRestoration } from '~/hooks/use-scroll-restoration'
import { SkipLinkTarget } from '~/ui/lib/SkipLink'
import { classed } from '~/util/classed'

export const PageContainer = classed.div`grid h-screen grid-cols-[14.25rem,1fr] grid-rows-[60px,1fr]`
export const PageContainer = classed.div`min-h-screen pt-[var(--navigation-height)] [overscroll-behavior-y:none]`

// TODO: this doesn't go tall enough on a tall screen to get the pagination bar to the bottom
// http://localhost:4000/projects/mock-project/disks
export function ContentPane() {
const ref = useRef<HTMLDivElement>(null)
useScrollRestoration(ref)
useScrollRestoration()
return (
<div ref={ref} className="flex flex-col overflow-auto" data-testid="scroll-container">
<div className="flex grow flex-col pb-8">
<div className="ml-[var(--sidebar-width)] flex min-h-full flex-col" id="content_pane">
<div className="flex grow flex-col pb-8 md-:pb-16">
<SkipLinkTarget />
<main className="[&>*]:gutter">
<Outlet />
</main>
</div>
<div className="sticky bottom-0 z-topBar shrink-0 justify-between overflow-hidden border-t bg-default border-secondary empty:border-t-0">
<div className="sticky bottom-0 z-topBar flex-shrink-0 justify-between overflow-hidden border-t bg-default border-secondary empty:border-t-0">
<Pagination.Target />
<PageActionsTarget />
</div>
Expand All @@ -42,7 +42,10 @@ export function ContentPane() {
* `<div>` because we don't need it.
*/
export const SerialConsoleContentPane = () => (
<div className="flex flex-col overflow-auto">
<div
className="ml-[var(--sidebar-width)] flex min-h-full flex-col overflow-auto"
id="content_pane"
>
<div className="flex grow flex-col">
<SkipLinkTarget />
<main className="[&>*]:gutter h-full">
Expand Down
2 changes: 1 addition & 1 deletion app/pages/project/instances/instance/SerialConsolePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export function SerialConsolePage() {
}, [canConnect])

return (
<div className="!mx-0 flex h-full max-h-[calc(100vh-60px)] !w-full flex-col">
<div className="!mx-0 flex h-screen max-h-[calc(100vh-60px)] !w-full flex-col">
<Link
to={pb.instance(instanceSelector)}
className="mx-3 mb-6 mt-3 flex h-10 shrink-0 items-center rounded px-3 bg-accent-secondary"
Expand Down
20 changes: 19 additions & 1 deletion app/ui/styles/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,29 @@

:root {
--content-gutter: 2.5rem;
--sidebar-width: 14.25rem;
--navigation-height: 3.75rem;
}

@media (max-width: 768px) {
:root {
--content-gutter: 1.5rem;
}
}

@layer base {
html,
body,
#root {
@apply min-h-screen;
}

.msw-banner #content_pane {
@apply pb-10;
}

body {
@apply overflow-y-hidden text-default bg-default;
@apply text-default bg-default;
font-family:
SuisseIntl,
-apple-system,
Expand Down
20 changes: 12 additions & 8 deletions test/e2e/scroll-restore.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,34 @@
*
* Copyright Oxide Computer Company
*/
import { expect, test, type Page } from './utils'
import { expect, expectVisible, test, type Page } from './utils'

async function expectScrollTop(page: Page, expected: number) {
const container = page.getByTestId('scroll-container')
const getScrollTop = () => container.evaluate((el: HTMLElement) => el.scrollTop)
await expect.poll(getScrollTop).toBe(expected)
await page.waitForFunction((expected) => window.scrollY === expected, expected)
}

async function scrollTo(page: Page, to: number) {
const container = page.getByTestId('scroll-container')
await container.evaluate((el: HTMLElement, to) => el.scrollTo(0, to), to)
await page.evaluate((y) => {
window.scrollTo(0, y)
}, to)
}

test('scroll restore', async ({ page }) => {
// open small window to make scrolling easier
await page.setViewportSize({ width: 800, height: 500 })
// open short window to make scrolling easier
// needs to be wide enough to not require the nav to be opened
await page.setViewportSize({ width: 1024, height: 500 })

// nav to disks and scroll it
await page.goto('/projects/mock-project/disks')
// this helps us wait till the page is ready to scroll
await expectVisible(page, ['role=heading[name*="Disks"]'])
await expectScrollTop(page, 0)
await scrollTo(page, 143)

// nav to snapshots
await page.getByRole('link', { name: 'Snapshots' }).click()
await expectVisible(page, ['role=heading[name*="Snapshots"]'])
await expect(page).toHaveURL('/projects/mock-project/snapshots')
await expectScrollTop(page, 0)

// go back to disks, scroll is restored, scroll it some more
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/z-index.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ test('Dropdown content can scroll off page and doesn’t hide TopBar', async ({

// scroll the page down just enough that the button and the top item are off
// screen, but the bottom item is not
await page.mouse.wheel(0, 480)
await page.mouse.wheel(0, 500)

// if we don't do this, the test doesn't wait long enough for the following
// assertions to become true
Expand Down
Loading