fix(Dialog): improve responsive presentation behavior - #5352
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR Modified ComponentsDialog (@astryxdesign/core) · View in Storybook
Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
left a comment
There was a problem hiding this comment.
Thanks, the fullscreen fade is a good fix. Four nits below — the RTL one I'd fix before merge.
[Reviewed by Robohands]
| paddingBlockEnd: | ||
| 'max(var(--container-padding-block-end), env(safe-area-inset-bottom, 0px))', | ||
| paddingInlineStart: | ||
| 'max(var(--container-padding-inline-start), env(safe-area-inset-left, 0px))', |
There was a problem hiding this comment.
In RTL this pads the wrong edge; the notch side stays unprotected.
| '@media (prefers-reduced-motion: reduce)': 'none', | ||
| }, | ||
| }, | ||
| fullscreenSafeArea: { |
There was a problem hiding this comment.
A theme setting dialog: {padding: 0} no longer wins here — the floor puts the inset back.
| expect(inlineStyle).toContain('--x-maxHeight: 70dvh'); | ||
| }); | ||
|
|
||
| it('uses a fullscreen-specific fade animation instead of centered dialog movement', () => { |
There was a problem hiding this comment.
Put the translate/scale back in the keyframes and this still passes.
|
|
||
| [fix] Clamp standard Dialog width to dynamic viewport space with token gutters, add safe-area/fullscreen sizing and fade-only fullscreen motion updates, add opt-in adaptive Dialog/BottomSheet recipes, and add explicit presentation comparison stories. | ||
|
|
||
| @rubycheung |
There was a problem hiding this comment.
@rubycheung — this credits a different account.
Summary
widthas the preferred surface width75vhto75dvh, add safe-area-aware padding for fullscreen Dialog content, and make fullscreen open with fade-only motion instead of the centered Dialog translate/scaleDialogAdaptivePresentationblock recipe showing anAdaptiveDialogwrapper: Dialog remains the default everywhere, and callers may explicitly opt into BottomSheet only for touch-oriented <=lg contextsResponsive and Interaction Readiness outcomes
Responsive layout
Core/Dialog -- Readiness / wide viewport; requested 400px/default width remains the preferred surface width.Core/Dialog -- Readiness / narrow viewport; standard Dialog now emitsmax-width: min(100%, calc(100dvw - var(--spacing-4) - var(--spacing-4))).Touch, pointer, and hover
Accessibility and interaction contracts
<dialog>,aria-modal,DialogHeaderlabeling, andalertdialogonly forpurpose="required"remain unchanged and covered by existing tests.prefers-reduced-motion: reduce.Mobile viewport constraints
maxHeightis now75dvh; standard Dialog width clamp uses100dvw.max(existing token padding, env(safe-area-inset-* , 0px)), preserving the spacing-token floor while honoring larger safe-area insets.dvhimproves dynamic viewport sizing where supported, and the adaptive recipe usesBottomSheet height="tall"for keyboard-heavy forms. This does not prove keyboard occlusion handling for every Dialog/browser/content combination.Presentation choice
Dialog remains the default component and default presentation. This PR does not add automatic BottomSheet behavior to Core Dialog and does not introduce a universal adaptive component.
The new recipe demonstrates component substitution only when a caller explicitly opts in:
touchPresentation="dialog"keeps Dialog even in touch-oriented <=lg contexts.touchPresentation="fullscreen"chooses fullscreen Dialog for touch-oriented <=lg contexts.touchPresentation="bottom-sheet"chooses BottomSheet only when(max-width: 1024px) and (pointer: coarse) and (hover: none)matches.presentationoverride says otherwise.presentation="dialog" | "fullscreen" | "bottom-sheet"provides a deterministic override for tests and unusual environments.isOpen,onOpenChange,purpose, and accessible label/title contracts are aligned across presentations.purposemapping controls swipe and scrim dismissal (formblocks swipe/scrim,infoallows them,requiredblocks implicit dismissal), so callers choose the adaptive presentation knowingly.Local Storybook review
Storybook is running locally on port 6007. Useful story IDs:
core-dialog--readiness-wide-viewportcore-dialog--readiness-narrow-viewportcore-dialog--mobile-presentation-keep-dialogcore-dialog--mobile-presentation-fullscreen-dialogcore-dialog--mobile-presentation-bottom-sheetValidation
All of these passed locally:
pnpm exec prettier --check packages/core/src/Dialog/Dialog.tsx packages/core/src/Dialog/Dialog.test.tsx apps/storybook/stories/Dialog.stories.tsx packages/cli/assets/templates/blocks/components/Dialog/DialogAdaptivePresentation.tsx packages/cli/assets/templates/blocks/components/Dialog/DialogAdaptivePresentation.doc.mjs .changeset/dialog-responsive-interaction.mdpnpm exec vitest run packages/core/src/Dialog/Dialog.test.tsx packages/core/src/Dialog/DialogHeader.test.tsx packages/core/src/AlertDialog/AlertDialog.test.tsxpnpm exec vitest run packages/cli/clients/cli/commands/dialog-adaptive-template.test.mjspnpm -F @astryxdesign/cli astryx template DialogAdaptivePresentationplus checks fortouchPresentation="bottom-sheet",TOUCH_ORIENTED_LG_QUERY, andpresentation proppnpm -F @astryxdesign/cli astryx component Dialog --densepnpm -F @astryxdesign/core typecheckpnpm -F @astryxdesign/core typecheck:docspnpm -F @astryxdesign/storybook typecheckpnpm -F @astryxdesign/cli typecheck:template-docspnpm exec eslint packages/core/src/Dialog/Dialog.tsx packages/core/src/Dialog/Dialog.test.tsx apps/storybook/stories/Dialog.stories.tsx packages/cli/assets/templates/blocks/components/Dialog/DialogAdaptivePresentation.tsx packages/cli/clients/cli/commands/dialog-adaptive-template.test.mjspnpm check:changesetspnpm check:repopnpm lab:readiness:checkgit diff --checkRelationship
Independent of AlertDialog #5343 and the Responsive and Interaction Readiness rubric #5351. Neither unmerged PR is a dependency.