Repository navigation
fix(frontend): fix the ResponsiveDialog layout, the header separator, the layout landmarks, and the inset width - #202
Merged
Merged
Conversation
Owner
Author
|
Review findings that this PR does not fix:
|
…ndmarks, and wide content These tests fail on main. A long ResponsiveDialog puts the title and the scrolled content under the close button, and its footer scrolls out of view. The header separator sits 6 px above the sidebar trigger. The router devtools add a contentinfo landmark. The sidebar links have no navigation landmark. A wide child at 1024 px makes the page scroll 256 px sideways. A test also tabs through a long form in the dialog and checks that the field in focus is not hidden. It passes on main and guards the sticky header and footer.
…d the inset width The vertical separator had self-stretch, so in the centered header row it started at the top. It now centers on the sidebar trigger. The router devtools rendered in a footer, which is a contentinfo landmark. They now render in a div. The sidebar links had no navigation landmark. They are now in a nav named Main. SidebarInset had no min-w-0. A wide child, such as a table in a box that scrolls, made the inset grow and the page scroll sideways. The inset now shrinks to the room next to the sidebar. Closes #160 Closes #161 Closes #162
The breadcrumb read the page name from a map keyed by the URL path. A route with params, such as /users/$id, never matched and showed "Page". The map is now keyed by the full path of the matched route. The keys are typed by the generated route paths, so a wrong key fails the build. Refs #159
…r of the content The close button was absolute over the content that scrolls. A long title ran under it, and the content scrolled under it. The header now sticks to the top of the content that scrolls, covers the close button, and the title leaves room for the button. The footer scrolled with the content, so a tall dialog hid its buttons. The footer now sticks to the bottom of the content that scrolls. When a field gets focus, the scroll box gets scroll padding of the header and footer heights, so the browser does not scroll the field under them. The dialog keeps its height. A close button row above the content would add 12 px on a phone and push Add Filter of the 320 x 780 date filter off the screen. Closes #152
…der and footer The scroll padding was set in the focus event. Chromium decides the scroll before that padding applies, so the first field that got focus after a scroll stayed under the header. The header and the footer now report their heights to the dialog with a ResizeObserver, and the scroll box always has scroll padding of those heights. The test now scrolls before the first Tab. The rule to put the header first and the footer last is now in their JSDoc.
The test name said that the wide content scrolls in its own box, but the test only checked that the page does not scroll sideways.
…ls under it At 320 px, the Created Date filter showed the border and focus ring of the date field around the title after a scroll. The header covered only the content box, so the ring showed in the side gutter. On a phone the header also ended at the bottom of the title box, and the text reaches about 3 px below it. The new test scrolls a field in focus under a short title and checks that only the header shows in the row of the title. It fails until the next commit.
…h the same height as main The header and the footer now cover the side gutters of the content that scrolls, so a focus ring there does not show next to them. The scroll box has the rounded corners of the dialog, so the header and the footer do not cover them. The header gets 0.5rem of padding below the title, and a negative margin of the same size, so the padding does not take space. It covers the close button and the content just below the title. The min height for the close button is gone, so a short desktop dialog has the height and the title position of main again. It was 6 px taller. The filter sheet spacing test measured from the bottom of the header box. It now measures from the title, as its name says. On main the two were the same.
ResponsiveDialog capped the sheet with a fixed 4.5rem for one toast, and index.css overrode that cap with --toast-reserve. The component now uses var(--toast-reserve, 4.5rem) and the max-height transition, and the override in index.css is gone. The drawer already works this way.
pdevito3
force-pushed
the
fix/ui-layout
branch
from
October 28, 2026 00:31
62e4f2c to
d53a150
Compare
Owner
Author
|
Independent review of the fix-forward commits: no high or medium findings. One low finding stays open:
A second low finding (the JSDoc gap below the header was too small for a focus ring) is fixed in 9659931. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The root layout and the ResponsiveDialog had layout and accessibility defects:
/users/$id(React Aria links cannot take route params, and the breadcrumb shows "Page" for a route with params #159).What changed
ResponsiveDialog: the header and footer stick to the top and bottom of the content that scrolls. They cover the full width, side gutters included, so a field that scrolls under them does not show next to them. The title leaves room for the close button. The header has 0.5 rem of background below the title that takes no space, so it covers the close button and a short dialog keeps its height.ResponsiveDialog: the header and footer report their heights with aResizeObserver. The scroll box has scroll padding of those heights, so Tab does not move a field under them.ResponsiveDialog: the bottom sheet cap usesvar(--toast-reserve, 4.5rem). The override of that cap inindex.cssis gone, so one place owns it.__root.tsx: the separator centers on the trigger. The devtools render in adiv. The breadcrumb looks up the page name by the path of the matched route, not by the URL. The keys are typed by the generated route paths, so a wrong key fails the build.app-sidebar.tsx: the sidebar links are in<nav aria-label="Main">.SidebarInset: addsmin-w-0, so it shrinks to the room next to the sidebar.Refs #152
Closes #160
Closes #161
Closes #162
Refs #159
Scope
ResponsiveDialogFooter, such as the text filter and the components page. It stays open for the date and multiselect filters. Their Add Filter buttons are in their own footers, so in a tall sheet they still scroll out of view. The same controls also show in the desktop submenu popover, where the sticky footer offsets do not fit. A fix needs a footer that knows its surface, and a design choice for the Select All and Add row of the multiselect on a phone.useHrefgets only thehref, so params inrouterOptionscannot make a correct<a href>. Options:createLink(RACLink). This is what the TanStack docs show. It is a new link API with typed params.hrefas a plain string. Callers build the path, and route typing is lost.routerOptionstake params. This type-checks, but the renderedhrefis wrong.useCloseOnScrollcloses the popover, and close commits the selection).allowsCustomValue, which keeps the text but changes what the field means. (C) a controlledinputValueat the call site. (D) report it to react-spectrum.main(fix(frontend): correct four small UI defects #132),min-h-screenin the route wrappers (fix(frontend): correct seven small UI defects #126), anditems-centeron the button size row incomponents.tsx.min-w-0fixes a wide child inside a box that scrolls. A wide child with no box that scrolls still makes the page wider. That is normal CSS.Tradeoffs
p-4of the content. If that padding changes, these must change too.Blast Radius
ResponsiveDialogwith aResponsiveDialogHeaderorResponsiveDialogFootergets the sticky header and footer. A short dialog has the same size and title position as on main: the Edit First Name filter at 1280 px is 266 px tall with the title at the same y, and each phone sheet has the height it has on main.SidebarInsetis used once, in__root.tsx.Verification
responsive-dialog.test.tsxat 1280 x 720 and 390 x 844: title, scroll, footer, and focus. A new test scrolls a field in focus under a short title and checks that only the header shows in the title row. It fails before the header fix (the side gutter at both sizes, and the field below the title on a phone) and passes now.app-shell.spec.ts: separator centered, no contentinfo from the devtools, links in the Main nav, no side scroll at 1024 px. The filter-demo and toast specs from fix(frontend): fix toast and sheet defects on a phone #203 pass. The filter sheet spacing test now measures from the title, as its name says.pnpm lint,pnpm build,pnpm test, andpnpm test:e2e --workers=2pass.Screenshots
Each image shows main, then this PR before the last fix, then this PR now.
Sheet and Drawer, now, light and dark (the same as before the last fix):
The app shell header, before and after: