Skip to content

Commit 93f206c

Browse files
benjaminleonardclaudedavid-crespo
authored
Responsive grid layout: fixed TopBar/Sidebar with document scroll (#3178)
Refactor of #2087 Switch from a CSS grid layout (where TopBar and Sidebar occupied grid cells alongside ContentPane) to fixed-position TopBar and Sidebar with document-level scrolling. This is the foundation for mobile/tablet support and likely the place that has the potential to cause the most issues so I'm separating the two. - Viewport meta tag: proper responsive width=device-width instead of fixed width=1050 - CSS: add --sidebar-width variable, responsive --content-gutter (smaller below 1000px), remove overflow-y-hidden on body, set height: 100% on html/body/#root - TopBar: single fixed element (was two Fragment children occupying grid cells) - Sidebar: fixed position with overflow scroll - ContentPane: margin-left offset for sidebar, document-level scroll via min-h-[calc(100vh-...)] - Scroll restoration: window.scrollY instead of container .scrollTop - PageSkeleton: matches new fixed layout structure - E2e: scroll-restore test updated for document scroll, expectScrollTop/scrollTo utils use window This PR should produce a console that is visually identical to the current one. --- Broader responsive changes will come in another PR, and are not a blocker for this. Todo - [x] Fix table dropdown overlapping header - [x] Double check modals are not causing layout shift --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
1 parent 03ff84e commit 93f206c

30 files changed

Lines changed: 506 additions & 219 deletions

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ Thumbs.db
3131
test-results/
3232
ci-e2e-traces/
3333
playwright-report/
34+
.e2e-logs/
3435
.vercel
3536

3637
# Visual regression snapshots

.oxlintrc.json

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,14 +98,21 @@
9898
"app/layouts/**/*",
9999
"app/forms/**/*",
100100
"*.config.ts",
101-
"*.config.mjs"
101+
"*.config.mjs",
102+
"test/e2e/compact-reporter.ts"
102103
],
103104
"rules": {
104105
"import/no-default-export": "off"
105106
}
106107
},
107108
{
108-
"files": ["**/*.spec.ts", "**/*.config.ts", "**/*.config.mjs", "tools/**/*"],
109+
"files": [
110+
"**/*.spec.ts",
111+
"**/*.config.ts",
112+
"**/*.config.mjs",
113+
"tools/**/*",
114+
"test/e2e/compact-reporter.ts"
115+
],
109116
"rules": {
110117
"import/no-nodejs-modules": "off"
111118
}

AGENTS.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
- Co-locate Vitest specs next to the code they cover; use Testing Library utilities (`render`, `renderHook`, `fireEvent`, fake timers) to assert observable output rather than implementation details (`app/ui/lib/FileInput.spec.tsx`, `app/hooks/use-pagination.spec.ts`).
3333
- For sweeping styling changes, coordinate with the visual regression harness and follow `test/visual/README.md` for the workflow.
3434
- Fix root causes of flaky timing rather than adding `sleep()` workarounds in tests.
35+
- Local Playwright runs write a compact plain-text report to `.e2e-logs/` (gitignored, one timestamped `.log` per run, last 10 kept) via the custom reporter at `test/e2e/compact-reporter.ts`. Top line is `status: ... total=N passed=N ...`; each failure is a `── UNEXPECTED|FLAKY file:line title` block followed by the error (ANSI stripped). Latest run: `ls .e2e-logs | tail -1` — Read it directly, no parsing needed.
3536

3637
# Data fetching pattern
3738

@@ -100,7 +101,7 @@
100101

101102
# Layout & accessibility
102103

103-
- Build pages inside the shared `PageContainer`/`ContentPane` so you inherit the skip link, sticky footer, pagination target, and scroll restoration tied to `#scroll-container` (`app/layouts/helpers.tsx`, `app/hooks/use-scroll-restoration.ts`).
104+
- Build pages inside the shared `PageContainer`/`ContentPane` so you inherit the skip link, sticky footer, pagination target, and scroll restoration (`app/layouts/helpers.tsx`, `app/hooks/use-scroll-restoration.ts`).
104105
- Surface page-level buttons and pagination via the `PageActions` and `Pagination` tunnels from `tunnel-rat`; anything rendered through `.In` lands in `.Target` automatically.
105106
- For global loading states, reuse `PageSkeleton`—it keeps the MSW banner and grid layout stable, and `skipPaths` lets you opt-out for routes with custom layouts (`app/components/PageSkeleton.tsx`).
106107
- Enforce accessibility at the type level: use `AriaLabel` type from `app/ui/util/aria.ts` which requires exactly one of `aria-label` or `aria-labelledby` on custom interactive components.

app/components/PageSkeleton.tsx

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,15 @@
66
* Copyright Oxide Computer Company
77
*/
88

9+
import cn from 'classnames'
910
import { useLocation } from 'react-router'
1011

11-
import { PageContainer } from '~/layouts/helpers'
12+
import {
13+
ContentPane,
14+
PageContainer,
15+
sidebarWrapperClass,
16+
topBarWrapperClass,
17+
} from '~/layouts/helpers'
1218
import { classed } from '~/util/classed'
1319

1420
import { MswBanner } from './MswBanner'
@@ -28,18 +34,22 @@ export function PageSkeleton({ skipPaths }: { skipPaths?: RegExp[] }) {
2834
<>
2935
{process.env.MSW_BANNER ? <MswBanner disableButton /> : null}
3036
<PageContainer>
31-
<div className="border-secondary flex items-center gap-2 border-r border-b p-3">
32-
<Block className="h-8 w-8" />
33-
<Block className="h-4 w-24" />
34-
</div>
35-
<div className="border-secondary flex items-center justify-between gap-2 border-b p-3">
36-
<Block className="h-4 w-24" />
37-
<div className="flex items-center gap-2">
38-
<Block className="h-6 w-16" />
39-
<Block className="h-6 w-32" />
37+
{/* TopBar */}
38+
<div className={topBarWrapperClass}>
39+
<div className="border-secondary flex items-center gap-2 border-r p-3">
40+
<Block className="h-8 w-8" />
41+
<Block className="h-4 w-24" />
42+
</div>
43+
<div className="flex items-center justify-between gap-2 p-3">
44+
<Block className="h-4 w-24" />
45+
<div className="flex items-center gap-2">
46+
<Block className="h-6 w-16" />
47+
<Block className="h-6 w-32" />
48+
</div>
4049
</div>
4150
</div>
42-
<div className="border-secondary border-r p-4">
51+
{/* Sidebar */}
52+
<div className={cn(sidebarWrapperClass, 'p-4')}>
4353
<Block className="mb-10 h-4 w-full" />
4454
<div className="mb-6 space-y-2">
4555
<Block className="h-4 w-32" />
@@ -52,7 +62,8 @@ export function PageSkeleton({ skipPaths }: { skipPaths?: RegExp[] }) {
5262
<Block className="h-4 w-14" />
5363
</div>
5464
</div>
55-
<div className="light:bg-raise" />
65+
{/* Content */}
66+
<ContentPane />
5667
</PageContainer>
5768
</>
5869
)

app/components/Sidebar.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { Action16Icon, Document16Icon } from '@oxide/design-system/icons/react'
1212

1313
import { useIsActivePath } from '~/hooks/use-is-active-path'
1414
import { openQuickActions } from '~/hooks/use-quick-actions'
15+
import { sidebarWrapperClass } from '~/layouts/helpers'
1516
import { Button } from '~/ui/lib/Button'
1617
import { Truncate } from '~/ui/lib/Truncate'
1718

@@ -62,7 +63,12 @@ const JumpToButton = () => {
6263

6364
export function Sidebar({ children }: { children: React.ReactNode }) {
6465
return (
65-
<div className="text-sans-md text-raise border-secondary flex flex-col border-r">
66+
<div
67+
className={cn(
68+
sidebarWrapperClass,
69+
'text-sans-md text-raise flex flex-col overflow-y-auto overscroll-none'
70+
)}
71+
>
6672
<div className="mx-3 mt-4">
6773
<JumpToButton />
6874
</div>

app/components/TopBar.tsx

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222

2323
import { useCrumbs } from '~/hooks/use-crumbs'
2424
import { useCurrentUser } from '~/hooks/use-current-user'
25+
import { topBarWrapperClass } from '~/layouts/helpers'
2526
import { useThemeStore, type Theme } from '~/stores/theme'
2627
import { buttonStyle } from '~/ui/lib/Button'
2728
import * as DropdownMenu from '~/ui/lib/DropdownMenu'
@@ -32,16 +33,12 @@ import { pb } from '~/util/path-builder'
3233

3334
export function TopBar({ systemOrSilo }: { systemOrSilo: 'system' | 'silo' }) {
3435
const { me } = useCurrentUser()
35-
// The height of this component is governed by the `PageContainer`
36-
// It's important that this component returns two distinct elements (wrapped in a fragment).
37-
// Each element will occupy one of the top column slots provided by `PageContainer`.
3836
return (
39-
<>
40-
<div className="border-secondary flex items-center border-r border-b px-2">
37+
<div className={topBarWrapperClass}>
38+
<div className="border-secondary flex items-center border-r px-2">
4139
<HomeButton level={systemOrSilo} />
4240
</div>
43-
{/* Height is governed by PageContainer grid */}
44-
<div className="bg-default border-secondary flex items-center justify-between gap-4 border-b px-3">
41+
<div className="flex items-center justify-between gap-4 px-3">
4542
<div className="flex flex-1 gap-2.5">
4643
<Breadcrumbs />
4744
</div>
@@ -50,7 +47,7 @@ export function TopBar({ systemOrSilo }: { systemOrSilo: 'system' | 'silo' }) {
5047
<UserMenu />
5148
</div>
5249
</div>
53-
</>
50+
</div>
5451
)
5552
}
5653

@@ -146,7 +143,7 @@ function UserMenu() {
146143
</span>
147144
</div>
148145
</DropdownMenu.Trigger>
149-
<DropdownMenu.Content gap={8}>
146+
<DropdownMenu.Content gap={8} zIndex="topBar">
150147
<DropdownMenu.LinkItem to={pb.profile()}>Settings</DropdownMenu.LinkItem>
151148
<ThemeSubmenu />
152149
<DropdownMenu.Item onSelect={() => logout.mutate({})} label="Sign out" />
@@ -238,7 +235,7 @@ function SiloSystemPicker({ level }: { level: 'silo' | 'system' }) {
238235
<SelectArrows6Icon className="text-quaternary ml-3 w-1.5!" aria-hidden />
239236
</div>
240237
</DropdownMenu.Trigger>
241-
<DropdownMenu.Content className="mt-2" anchor="bottom start">
238+
<DropdownMenu.Content className="mt-2" anchor="bottom start" zIndex="topBar">
242239
<SystemSiloItem to={pb.silos()} label="System" isSelected={level === 'system'} />
243240
<SystemSiloItem to={pb.projects()} label="Silo" isSelected={level === 'silo'} />
244241
</DropdownMenu.Content>

app/hooks/use-scroll-restoration.ts

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,20 +18,30 @@ function setScrollPosition(key: string, pos: number) {
1818
}
1919

2020
/**
21-
* Given a ref to a scrolling container element, keep track of its scroll
22-
* position before navigation and restore it on return (e.g., back/forward nav).
23-
* Note that `location.key` is used in the cache key, not `location.pathname`,
24-
* so the same path navigated to at different points in the history stack will
25-
* not share the same scroll position.
21+
* Keep track of window scroll position before navigation and restore it on
22+
* return (e.g., back/forward nav). Note that `location.key` is used in the
23+
* cache key, not `location.pathname`, so the same path navigated to at
24+
* different points in the history stack will not share the same scroll position.
25+
*
26+
* We tried RR's built-in `<ScrollRestoration />` and it didn't work — on
27+
* back/forward nav, `window.scrollTo` was called with the right value but the
28+
* document was still at viewport height at that moment, so the scroll got
29+
* clamped to 0. We're not sure why; a theory is that RR restores in a
30+
* `useLayoutEffect` which fires before some later render expands the content,
31+
* and our `useEffect` after paint happens to catch that later render.
2632
*/
27-
export function useScrollRestoration(container: React.RefObject<HTMLElement | null>) {
33+
export function useScrollRestoration() {
2834
const key = `scroll-position-${useLocation().key}`
2935
const { state } = useNavigation()
3036
useEffect(() => {
37+
// opt out of the browser's native scroll restoration so it doesn't jump
38+
// the still-visible old page to the new page's saved position on POP,
39+
// before the new route's loader resolves. We restore manually below.
40+
window.history.scrollRestoration = 'manual'
3141
if (state === 'loading') {
32-
setScrollPosition(key, container.current?.scrollTop ?? 0)
42+
setScrollPosition(key, window.scrollY)
3343
} else if (state === 'idle') {
34-
container.current?.scrollTo(0, getScrollPosition(key))
44+
window.scrollTo(0, getScrollPosition(key))
3545
}
36-
}, [key, state, container])
46+
}, [key, state])
3747
}

app/layouts/helpers.tsx

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
*
66
* Copyright Oxide Computer Company
77
*/
8-
import { useRef } from 'react'
98
import { Outlet } from 'react-router'
109

1110
import { PageActionsTarget } from '~/components/PageActions'
@@ -14,18 +13,18 @@ import { useScrollRestoration } from '~/hooks/use-scroll-restoration'
1413
import { SkipLinkTarget } from '~/ui/lib/SkipLink'
1514
import { classed } from '~/util/classed'
1615

17-
export const PageContainer = classed.div`grid h-screen grid-cols-[14.25rem_1fr] grid-rows-[var(--top-bar-height)_1fr]`
16+
export const PageContainer = classed.div`min-h-full pt-(--top-bar-height)`
17+
18+
// shared with PageSkeleton so the skeleton doesn't drift from the real layout
19+
export const topBarWrapperClass =
20+
'bg-default border-secondary fixed top-0 right-0 left-0 z-(--z-top-bar) grid h-(--top-bar-height) grid-cols-[var(--sidebar-width)_1fr] border-b'
21+
export const sidebarWrapperClass =
22+
'border-secondary fixed top-(--top-bar-height) bottom-0 left-0 w-(--sidebar-width) border-r'
1823

1924
export function ContentPane() {
20-
const ref = useRef<HTMLDivElement>(null)
21-
useScrollRestoration(ref)
25+
useScrollRestoration()
2226
return (
23-
<div
24-
ref={ref}
25-
className="light:bg-raise flex flex-col overflow-auto"
26-
id="scroll-container"
27-
data-testid="scroll-container"
28-
>
27+
<div className="light:bg-raise ml-(--sidebar-width) flex min-h-[calc(100vh-var(--top-bar-height))] flex-col">
2928
<div className="flex grow flex-col pb-8">
3029
<SkipLinkTarget />
3130
<main className="*:gutter">
@@ -47,12 +46,10 @@ export function ContentPane() {
4746
* `<div>` because we don't need it.
4847
*/
4948
export const SerialConsoleContentPane = () => (
50-
<div className="flex flex-col overflow-auto">
51-
<div className="flex grow flex-col">
52-
<SkipLinkTarget />
53-
<main className="*:gutter h-full">
54-
<Outlet />
55-
</main>
56-
</div>
49+
<div className="ml-(--sidebar-width) flex h-[calc(100vh-var(--top-bar-height))] flex-col overflow-hidden">
50+
<SkipLinkTarget />
51+
<main className="*:gutter h-full">
52+
<Outlet />
53+
</main>
5754
</div>
5855
)

app/table/QueryTable.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ function useScrollReset(triggerDep: string | undefined) {
4646
const resetRequested = useRef(false)
4747
useEffect(() => {
4848
if (resetRequested.current) {
49-
document.querySelector('#scroll-container')?.scrollTo(0, 0)
49+
window.scrollTo(0, 0)
5050
resetRequested.current = false
5151
}
5252
}, [triggerDep])

app/table/columns/action-col.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,11 @@ export const RowActions = ({ id, copyIdLabel = 'Copy ID', actions }: RowActionsP
9292
<More12Icon />
9393
</DropdownMenu.Trigger>
9494
{/* offset moves menu in from the right so it doesn't align with the table border */}
95-
<DropdownMenu.Content anchor={{ to: 'bottom end', offset: -6 }} className="-mt-2">
95+
<DropdownMenu.Content
96+
anchor={{ to: 'bottom end', offset: -6 }}
97+
className="-mt-2"
98+
collisionPadding={{ bottom: 56 }}
99+
>
96100
{id && <CopyIdItem id={id} label={copyIdLabel} />}
97101
{actions?.map(({ className, ...action }) =>
98102
'to' in action ? (

0 commit comments

Comments
 (0)