feat(layer-dialog): add responsive layer dialog - #793
Open
mattrothenberg wants to merge 14 commits into
Open
Conversation
Collaborator
Author
|
/bonk give me a thorough code review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
Contributor
|
I'm Bonk, and I've done a quick review of your PR. 2 findings posted inline. No additional actionable findings. Review notes
|
commit: |
Contributor
|
I'm Bonk, and I've done a quick review of your PR. Posted 1 new inline finding. Review notes
|
Contributor
📐 Bundle SizeConsumer fixtures bundled against
npm tarball: 534 files, 1.47 MB packed, 6.84 MB unpacked.
|
Contributor
Visual Regression Report — 32 changed, 13 unchanged32 screenshot(s) with visual changes: Button / Basic243 px (0.27%) changed
Button / Variant: Primary473 px (0.53%) changed
Button / Variant: Ghost101 px (0.11%) changed
Button / Sizes1,697 px (1.92%) changed
Button / With Icon819 px (0.92%) changed
Button / Loading State195 px (0.22%) changed
Button / Disabled State232 px (0.26%) changed
Button / Title848 px (0.96%) changed
Button / Disabled Link332 px (0.37%) changed
Dialog / Dialog With Actions401 px (0.45%) changed
Dialog / Dialog Sizes3,060 px (3.46%) changed
Dialog / Dialog Alert1,983 px (2.24%) changed
Dialog / Dialog Confirmation855 px (0.97%) changed
Dialog / Custom Max Width303 px (0.34%) changed
Dialog / Dialog With Select383 px (0.43%) changed
Dialog / Dialog With Dropdown253 px (0.29%) changed
Dialog (Open)0 px (0%) changed
Select / Select Basic859 px (0.97%) changed
Select / Select Sizes3,108 px (1.91%) changed
Select / Select Placement3,530 px (2.44%) changed
Select / Select Dynamic Placement2,608 px (2.51%) changed
Select / Select Without Label758 px (0.86%) changed
Select / Select With Description1,687 px (1.62%) changed
Select / Select With Error1,730 px (1.66%) changed
Select / Select With Tooltip196 px (0.22%) changed
Select / Select Custom Rendering234 px (0.26%) changed
Select / Select Loading3,161 px (1.79%) changed
Select / Select Disabled Options1,217 px (1.37%) changed
Select / Select Disabled Items103 px (0.12%) changed
Select / Select Grouped937 px (1.06%) changed
Select / Select Grouped With Disabled1,414 px (1.6%) changed
Select (Open)3,292 px (0.01%) changed
13 screenshot(s) unchanged
Generated by Kumo Visual Regression |
- Add LayerDialog.Description, rendered beneath the title inside the sticky title frame and wired as the dialog's accessible description. Without it, the body copy describes the dialog so alerts still announce consequences. - Only block user-initiated dismissal reasons, so programmatic closes via actionsRef or a controlled `open` work for alerts and while dismissDisabled. Escape now cancels alerts, matching Base UI's AlertDialog. - Pass alert state as a prop instead of inheriting context, so a plain Root nested inside an Alert no longer becomes an alert. - Move Body's title/close-button slots to a private context so they are not part of the public props. - Resolve size and verticalAlign through resolveVariant to avoid crashes on unknown values. - Replace the nonexistent shadow-m utility with an explicit shadow. - Export LayerDialogProps as a real type and categorize the component as Overlay so the registry documents the root props. - Add Installation, Usage, and API Reference sections to the docs page and move demo intro copy into Description. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
The ScrollArea viewport is always `overflow: scroll`, so WebKit rubber-bands it even with no overflow. `overscroll-contain` only stops chaining; switch to `overscroll-none` so the element's own bounce is disabled too. Drawer swipe detection reads scroll height, not overscroll-behavior, so swipe-to-close is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
The popup hardcoded `max-h-[calc(100dvh-3rem)]` while the top-aligned variant added 4rem of viewport padding, so a max-height dialog could extend past the bottom edge. Follow Base UI's inside-scroll pattern: each verticalAlign variant reserves vertical padding on the viewport and the popup fills it with `max-h-full`, so the cap can never drift from the alignment. Mobile sheets keep their 85dvh cap. Add a test pinning the contract and a "Maximum height" docs section with a tall-content demo for both placements. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
Collaborator
Author
|
One design detail I've been noodling on is the 'sticky' behavior for the title + description. From a spacing perspective, I think it looks way nicer for the title + description to be visually grouped together, but it adds a lot of height once it becomes sticky. Could we do something fancy like this to de-emphasize the description? I could also be overthinking this and we should just put description in the body content and not worry about this :) Particularly curious what you think @hkfoster FancyCleanShot.2026-09-09.at.22.00.26.mp4Less FancyCleanShot.2026-09-09.at.22.08.07.mp4 |
Once the body scrolls past the header threshold, the description collapses under the title (grid-template-rows 1fr -> 0fr plus opacity) and unfolds again at the top. It stays in the DOM so aria-describedby keeps resolving. Collapsing gives the description's height to the scroll viewport, which shrinks the scroll range. If the content only barely overflows, that would clamp scrollTop under the threshold and re-expand in a loop, so the handler only condenses when enough overflow survives the collapse. Measured in headless Chromium at 120Hz with 600 rows each holding an Input: collapse/expand max frame 9.1ms vs 8.8ms idle, zero frames over 20ms. At 3000 rows the only 16.7ms frames match plain scrolling. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
Alerts no longer force a red primary action. Actions.Primary accepts variant="primary" (default) or variant="destructive", so a critical but non-destructive confirmation keeps brand styling while deletes opt into danger explicitly. Documented in the API reference alongside the existing loading prop. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
The X button's accessible name and the footer dismiss text were hardcoded English. Content now takes closeLabel for the X, and Actions' dismissLabel is plain text instead of a close/cancel enum, defaulting to Close (Cancel inside Alert). Both are documented under Localization. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DVfWmxDS5smTdZh5mx9cbq
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
1 participant
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.
Summary
LayerDialogcomponent built on Base UI Draweralertdialogsemantics, and custom portal-container supportValidation
pnpm --filter @cloudflare/kumo lintpnpm --filter @cloudflare/kumo test— 55 files and 1,320 tests passedpnpm --filter @cloudflare/kumo buildpnpm --filter @cloudflare/kumo-docs-astro typecheck— 0 errors