From 37c5f788d17d7518d0c111cf444b258fd972b8f3 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 16 Sep 2024 15:28:10 -0500 Subject: [PATCH 1/7] extract non-grid layout from responsive console PR --- app/layouts/helpers.tsx | 14 +++++++++----- app/ui/styles/index.css | 25 ++++++++++++++++++++++++- test/e2e/scroll-restore.e2e.ts | 20 ++++++++++++-------- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index fc6173bd2b..725b08c3a1 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -14,20 +14,24 @@ 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`h-full pt-[var(--navigation-height)] [overscroll-behavior-y:none]` export function ContentPane() { const ref = useRef(null) useScrollRestoration(ref) return ( -
-
+
+
-
+
@@ -42,7 +46,7 @@ export function ContentPane() { * `
` because we don't need it. */ export const SerialConsoleContentPane = () => ( -
+
diff --git a/app/ui/styles/index.css b/app/ui/styles/index.css index 5746fa8af6..d853d64389 100644 --- a/app/ui/styles/index.css +++ b/app/ui/styles/index.css @@ -44,11 +44,34 @@ :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 { + overscroll-behavior-y: none; + @apply h-full; + } + + #content_pane { + @apply min-h-full; + } + + .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, diff --git a/test/e2e/scroll-restore.e2e.ts b/test/e2e/scroll-restore.e2e.ts index 318b2e8a59..4f85aeb974 100644 --- a/test/e2e/scroll-restore.e2e.ts +++ b/test/e2e/scroll-restore.e2e.ts @@ -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 From 9ba24f14783e5d61b13e5c6062a80c0c3ce8d427 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 16 Sep 2024 15:57:34 -0500 Subject: [PATCH 2/7] fixes --- app/components/Sidebar.tsx | 2 +- app/components/TopBar.tsx | 11 +++++------ app/layouts/helpers.tsx | 6 +----- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/app/components/Sidebar.tsx b/app/components/Sidebar.tsx index 0cc380ca40..c4705f50e2 100644 --- a/app/components/Sidebar.tsx +++ b/app/components/Sidebar.tsx @@ -56,7 +56,7 @@ const JumpToButton = () => { export function Sidebar({ children }: { children: React.ReactNode }) { return ( -
+
diff --git a/app/components/TopBar.tsx b/app/components/TopBar.tsx index 4c5e2e2366..c88e0764ae 100644 --- a/app/components/TopBar.tsx +++ b/app/components/TopBar.tsx @@ -30,13 +30,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 ( - <> -
+
+
{cornerPicker}
- {/* Height is governed by PageContainer grid */} - {/* shrink-0 is needed to prevent getting squished by body content */} -
+
+ {/* shrink-0 is needed to prevent getting squished by body content */}
{otherPickers}
@@ -68,6 +67,6 @@ export function TopBar({ children }: { children: React.ReactNode }) {
- +
) } diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 725b08c3a1..ff38694d4e 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -20,11 +20,7 @@ export function ContentPane() { const ref = useRef(null) useScrollRestoration(ref) return ( -
+
From 2cae1990a6ffe5b0a58033199b05cad6af68b71b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 16 Sep 2024 16:06:25 -0500 Subject: [PATCH 3/7] switch to RR ScrollRestoration (doesn't work though) --- app/hooks/use-scroll-restoration.ts | 37 ----------------------------- app/layouts/AuthenticatedLayout.tsx | 3 ++- app/layouts/helpers.tsx | 8 ++----- app/ui/styles/index.css | 4 ---- 4 files changed, 4 insertions(+), 48 deletions(-) delete mode 100644 app/hooks/use-scroll-restoration.ts diff --git a/app/hooks/use-scroll-restoration.ts b/app/hooks/use-scroll-restoration.ts deleted file mode 100644 index 8dbd8a5848..0000000000 --- a/app/hooks/use-scroll-restoration.ts +++ /dev/null @@ -1,37 +0,0 @@ -/* - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, you can obtain one at https://mozilla.org/MPL/2.0/. - * - * Copyright Oxide Computer Company - */ -import { useEffect } from 'react' -import { useLocation, useNavigation } from 'react-router-dom' - -function getScrollPosition(key: string) { - const pos = window.sessionStorage.getItem(key) - return Number(pos) || 0 -} - -function setScrollPosition(key: string, pos: number) { - window.sessionStorage.setItem(key, pos.toString()) -} - -/** - * 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. - */ -export function useScrollRestoration(container: React.RefObject) { - const key = `scroll-position-${useLocation().key}` - const { state } = useNavigation() - useEffect(() => { - if (state === 'loading') { - setScrollPosition(key, container.current?.scrollTop ?? 0) - } else if (state === 'idle') { - container.current?.scrollTo(0, getScrollPosition(key)) - } - }, [key, state, container]) -} diff --git a/app/layouts/AuthenticatedLayout.tsx b/app/layouts/AuthenticatedLayout.tsx index b4cbc16682..e47a1b63b8 100644 --- a/app/layouts/AuthenticatedLayout.tsx +++ b/app/layouts/AuthenticatedLayout.tsx @@ -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' @@ -43,6 +43,7 @@ export function AuthenticatedLayout() { <> + ) } diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index ff38694d4e..63ea2e1b1a 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -5,22 +5,18 @@ * * Copyright Oxide Computer Company */ -import { useRef } from 'react' import { Outlet } from 'react-router-dom' import { PageActionsTarget } from '~/components/PageActions' import { Pagination } from '~/components/Pagination' -import { useScrollRestoration } from '~/hooks/use-scroll-restoration' import { SkipLinkTarget } from '~/ui/lib/SkipLink' import { classed } from '~/util/classed' export const PageContainer = classed.div`h-full pt-[var(--navigation-height)] [overscroll-behavior-y:none]` export function ContentPane() { - const ref = useRef(null) - useScrollRestoration(ref) return ( -
+
@@ -42,7 +38,7 @@ export function ContentPane() { * `
` because we don't need it. */ export const SerialConsoleContentPane = () => ( -
+
diff --git a/app/ui/styles/index.css b/app/ui/styles/index.css index d853d64389..2ee20cf520 100644 --- a/app/ui/styles/index.css +++ b/app/ui/styles/index.css @@ -62,10 +62,6 @@ @apply h-full; } - #content_pane { - @apply min-h-full; - } - .msw-banner #content_pane { @apply pb-10; } From 9aa06c2e34be7ff011d764bf5f34c1b05ca7c87a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 17 Sep 2024 14:47:53 -0500 Subject: [PATCH 4/7] all dropdowns are modal=false --- app/ui/lib/DropdownMenu.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/ui/lib/DropdownMenu.tsx b/app/ui/lib/DropdownMenu.tsx index 929296262a..4a95d5521e 100644 --- a/app/ui/lib/DropdownMenu.tsx +++ b/app/ui/lib/DropdownMenu.tsx @@ -15,7 +15,7 @@ import { type DropdownMenuItemProps, } from '@radix-ui/react-dropdown-menu' import cn from 'classnames' -import { forwardRef, type ForwardedRef } from 'react' +import { forwardRef, type ForwardedRef, type ReactNode } from 'react' import { Link } from 'react-router-dom' type DivRef = ForwardedRef @@ -25,7 +25,7 @@ type DivRef = ForwardedRef type LinkitemProps = Omit & { to: string } export const DropdownMenu = { - Root, + Root: ({ children }: { children: ReactNode }) => {children}, Trigger, Portal, // don't need to forward ref here for a particular reason but Radix gives a From f4402f1edd93d77d159eae69d724684161dcc076 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 19 Sep 2024 15:16:33 -0500 Subject: [PATCH 5/7] fix z-index test --- test/e2e/z-index.e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/z-index.e2e.ts b/test/e2e/z-index.e2e.ts index 6379685bc3..a76538da00 100644 --- a/test/e2e/z-index.e2e.ts +++ b/test/e2e/z-index.e2e.ts @@ -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 From 5cff8829a412ecee9da4011fde3bab1b9f27b1e0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 19 Sep 2024 15:49:47 -0500 Subject: [PATCH 6/7] fix serial console layout, discover bug on tall screens --- app/layouts/helpers.tsx | 9 +++++++-- .../project/instances/instance/SerialConsolePage.tsx | 2 +- app/ui/styles/index.css | 3 +-- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 3f30dec92c..46557799be 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -12,8 +12,10 @@ import { Pagination } from '~/components/Pagination' import { SkipLinkTarget } from '~/ui/lib/SkipLink' import { classed } from '~/util/classed' -export const PageContainer = classed.div`h-full pt-[var(--navigation-height)] [overscroll-behavior-y:none]` +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() { return (
@@ -38,7 +40,10 @@ export function ContentPane() { * `
` because we don't need it. */ export const SerialConsoleContentPane = () => ( -
+
diff --git a/app/pages/project/instances/instance/SerialConsolePage.tsx b/app/pages/project/instances/instance/SerialConsolePage.tsx index a6f3bdacfe..bf536f3450 100644 --- a/app/pages/project/instances/instance/SerialConsolePage.tsx +++ b/app/pages/project/instances/instance/SerialConsolePage.tsx @@ -135,7 +135,7 @@ export function SerialConsolePage() { }, [canConnect]) return ( -
+
Date: Thu, 19 Sep 2024 16:08:10 -0500 Subject: [PATCH 7/7] bring back our own scroll restoration hook, use window instead of ref --- app/hooks/use-scroll-restoration.ts | 36 +++++++++++++++++++++++++++++ app/layouts/helpers.tsx | 2 ++ 2 files changed, 38 insertions(+) create mode 100644 app/hooks/use-scroll-restoration.ts diff --git a/app/hooks/use-scroll-restoration.ts b/app/hooks/use-scroll-restoration.ts new file mode 100644 index 0000000000..5653257fe9 --- /dev/null +++ b/app/hooks/use-scroll-restoration.ts @@ -0,0 +1,36 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import { useEffect } from 'react' +import { useLocation, useNavigation } from 'react-router-dom' + +function getScrollPosition(key: string) { + const pos = window.sessionStorage.getItem(key) + return Number(pos) || 0 +} + +function setScrollPosition(key: string, pos: number) { + window.sessionStorage.setItem(key, pos.toString()) +} + +/** + * 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() { + const key = `scroll-position-${useLocation().key}` + const { state } = useNavigation() + useEffect(() => { + if (state === 'loading') { + setScrollPosition(key, window.scrollY ?? 0) + } else if (state === 'idle') { + window.scrollTo(0, getScrollPosition(key)) + } + }, [key, state]) +} diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 46557799be..fc0d1e376a 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -9,6 +9,7 @@ import { Outlet } from 'react-router-dom' import { PageActionsTarget } from '~/components/PageActions' import { Pagination } from '~/components/Pagination' +import { useScrollRestoration } from '~/hooks/use-scroll-restoration' import { SkipLinkTarget } from '~/ui/lib/SkipLink' import { classed } from '~/util/classed' @@ -17,6 +18,7 @@ export const PageContainer = classed.div`min-h-screen pt-[var(--navigation-heigh // 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() { + useScrollRestoration() return (