From fd960d1d376bc0ba05cd8789d4e72090edd53276 Mon Sep 17 00:00:00 2001 From: Max Yinger Date: Tue, 15 Sep 2026 11:51:17 -0600 Subject: [PATCH 01/14] feat(ui): rebuild Mosaic Dialog on bring-your-own surfaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the `prompt` size. `DialogSize` is now `card | profile`, the default is `card`, and `role='alertdialog'` no longer forces a size — the role decides the dismissal policy and nothing else. Every dialog brings its own surface, so the popup paints nothing: `Dialog.Title`, `Dialog.Description`, `Dialog.Actions` and `Dialog.Confirm` are gone, along with `createConfirmHandle` and `useConfirmedClose`, and `Card.Title` / `Card.Description` / `Card.Footer` do that work instead. The phone-band sheet the `prompt` carried implicitly becomes an explicit axis: `compactPlacement='sheet'` on `Dialog.Popup`. It centres what it holds, since the band runs to 48rem while a Card caps at 26.25rem and would otherwise sit against one edge. `Card.Header` withholds its dismiss inside an alert dialog — a corner X answers the question by leaving — reading the role from `DialogContext`, which the popup now publishes. `Confirmation` takes the role it was blocked from, as a sheet. Co-Authored-By: Claude Opus 5 (1M context) --- .changeset/mosaic-dialog-drop-prompt.md | 2 + .../ui/src/mosaic/__tests__/props.test.ts | 2 +- .../blocks/confirmation/confirmation.test.tsx | 22 +- .../blocks/confirmation/confirmation.tsx | 10 +- .../src/mosaic/components/card/card.test.tsx | 9 +- .../ui/src/mosaic/components/card/card.tsx | 10 +- .../components/dialog/alert-dialog.test.tsx | 214 +++++------ .../components/dialog/confirm-handle.ts | 104 ----- .../mosaic/components/dialog/confirm.test.tsx | 354 ------------------ .../mosaic/components/dialog/dialog.styles.ts | 312 +++++++-------- .../mosaic/components/dialog/dialog.test.tsx | 192 ++++++---- .../src/mosaic/components/dialog/dialog.tsx | 268 ++++++------- .../ui/src/mosaic/components/dialog/index.ts | 9 +- .../components/dialog/use-confirmed-close.ts | 77 ---- .../hooks/useAccessibleDescriptionWarning.ts | 8 +- .../mosaic/hooks/useAccessibleNameWarning.ts | 12 +- packages/ui/src/mosaic/styles/index.ts | 10 +- 17 files changed, 545 insertions(+), 1070 deletions(-) create mode 100644 .changeset/mosaic-dialog-drop-prompt.md delete mode 100644 packages/ui/src/mosaic/components/dialog/confirm-handle.ts delete mode 100644 packages/ui/src/mosaic/components/dialog/confirm.test.tsx delete mode 100644 packages/ui/src/mosaic/components/dialog/use-confirmed-close.ts diff --git a/.changeset/mosaic-dialog-drop-prompt.md b/.changeset/mosaic-dialog-drop-prompt.md new file mode 100644 index 00000000000..a845151cc84 --- /dev/null +++ b/.changeset/mosaic-dialog-drop-prompt.md @@ -0,0 +1,2 @@ +--- +--- diff --git a/packages/ui/src/mosaic/__tests__/props.test.ts b/packages/ui/src/mosaic/__tests__/props.test.ts index b07f279c07f..675947eef10 100644 --- a/packages/ui/src/mosaic/__tests__/props.test.ts +++ b/packages/ui/src/mosaic/__tests__/props.test.ts @@ -63,7 +63,7 @@ describe('mergeStyleProps', () => { }); it("merges a render source's className/style out of the incoming prop bag", () => { - // `Dialog.Title render={}` hands Heading the title's merged pair through its + // `Card.Title render={}` hands Heading the title's merged pair through its // props; passing that bag last must merge them rather than clobber the part's own class. const rest = { className: 'cl-dialog-title', style: { color: 'red' }, id: 'title' }; const merged = mergeStyleProps({ className: 'cl-heading' }, { className: 'x1', style: { margin: 0 } }, rest); diff --git a/packages/ui/src/mosaic/blocks/confirmation/confirmation.test.tsx b/packages/ui/src/mosaic/blocks/confirmation/confirmation.test.tsx index d63c261ca3c..e46f6b571af 100644 --- a/packages/ui/src/mosaic/blocks/confirmation/confirmation.test.tsx +++ b/packages/ui/src/mosaic/blocks/confirmation/confirmation.test.tsx @@ -29,7 +29,7 @@ describe('Confirmation', () => { it('renders nothing until the caller opens it', () => { renderBlock({ open: false }); - expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument(); }); it('asks to open from the trigger', async () => { @@ -61,7 +61,7 @@ describe('Confirmation', () => { ), }); - expect(screen.getByRole('dialog')).toHaveAccessibleDescription( + expect(screen.getByRole('alertdialog')).toHaveAccessibleDescription( 'preston@clerk.dev will be removed from this account.', ); expect(screen.getByText('preston@clerk.dev').tagName).toBe('STRONG'); @@ -127,7 +127,7 @@ describe('Confirmation with a handle', () => { it('renders nothing until opened through the handle', () => { renderWithHandle(); - expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument(); }); it('opens with a payload and renders the copy from it', () => { @@ -137,8 +137,8 @@ describe('Confirmation with a handle', () => { handle.open(preston); }); - expect(screen.getByRole('dialog')).toHaveAccessibleName('Remove member'); - expect(screen.getByRole('dialog')).toHaveAccessibleDescription( + expect(screen.getByRole('alertdialog')).toHaveAccessibleName('Remove member'); + expect(screen.getByRole('alertdialog')).toHaveAccessibleDescription( 'Preston Booth will be removed from the organization.', ); expect(screen.getByText('Preston Booth').tagName).toBe('STRONG'); @@ -156,7 +156,7 @@ describe('Confirmation with a handle', () => { await user.click(removeButton()); expect(onConfirm).toHaveBeenCalledWith(preston); - await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + await waitFor(() => expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument()); }); it('stays inert and open while the action is pending', async () => { @@ -170,7 +170,7 @@ describe('Confirmation with a handle', () => { expect(removeButton()).toHaveAttribute('aria-busy', 'true'); await user.keyboard('{Escape}'); - expect(screen.getByRole('dialog')).toBeInTheDocument(); + expect(screen.getByRole('alertdialog')).toBeInTheDocument(); }); it('keeps the dialog open and explains a failed attempt', async () => { @@ -183,7 +183,7 @@ describe('Confirmation with a handle', () => { await user.click(removeButton()); await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('Preston Booth is the last admin.')); - expect(screen.getByRole('dialog')).toBeInTheDocument(); + expect(screen.getByRole('alertdialog')).toBeInTheDocument(); expect(removeButton()).not.toHaveAttribute('aria-busy', 'true'); }); @@ -197,12 +197,12 @@ describe('Confirmation with a handle', () => { await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()); await user.click(screen.getByRole('button', { name: 'Cancel' })); - await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + await waitFor(() => expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument()); act(() => { handle.open(preston); }); - expect(screen.getByRole('dialog')).toBeInTheDocument(); + expect(screen.getByRole('alertdialog')).toBeInTheDocument(); expect(screen.queryByRole('alert')).not.toBeInTheDocument(); }); @@ -217,7 +217,7 @@ describe('Confirmation with a handle', () => { handle.close(); }); - await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + await waitFor(() => expect(screen.queryByRole('alertdialog')).not.toBeInTheDocument()); expect(handle.isOpen).toBe(false); }); }); diff --git a/packages/ui/src/mosaic/blocks/confirmation/confirmation.tsx b/packages/ui/src/mosaic/blocks/confirmation/confirmation.tsx index f3d1f86789c..d2f9d9f02eb 100644 --- a/packages/ui/src/mosaic/blocks/confirmation/confirmation.tsx +++ b/packages/ui/src/mosaic/blocks/confirmation/confirmation.tsx @@ -27,7 +27,7 @@ function ConfirmationCard({ errorMessage, }: ConfirmationCardProps) { return ( - + @@ -176,7 +176,7 @@ function HandleConfirmation({ return ( = ConfirmationControlledProps | * Confirmation dialog for a destructive action that is worth a second look but not worth * making the user type for. Use `Destructive` for the actions that are. * + * An `alertdialog`: it announces as an interruption, an outside press cannot answer it, and the + * card withholds its corner dismiss — the footer is the only way out. Under the phone band it + * arrives as a bottom sheet, within reach of the thumb that has to answer it. + * * Two forms. Controlled: the caller owns `open`, `isConfirming`, and `errorMessage`, and the * block holds nothing of its own. With a `handle`: the block owns all three. Mount it once, * open it from anywhere with `handle.open(payload)`, and derive the copy and the action from diff --git a/packages/ui/src/mosaic/components/card/card.test.tsx b/packages/ui/src/mosaic/components/card/card.test.tsx index bf6cb94aca5..7f72b044d46 100644 --- a/packages/ui/src/mosaic/components/card/card.test.tsx +++ b/packages/ui/src/mosaic/components/card/card.test.tsx @@ -277,7 +277,14 @@ describe('Mosaic Card', () => { Open - Review terms + + + Review terms + + , ); diff --git a/packages/ui/src/mosaic/components/card/card.tsx b/packages/ui/src/mosaic/components/card/card.tsx index 8b4c6a0f1ac..b914d4cd5da 100644 --- a/packages/ui/src/mosaic/components/card/card.tsx +++ b/packages/ui/src/mosaic/components/card/card.tsx @@ -66,8 +66,8 @@ const Root = React.forwardRef(function CardRoot( /** * Dismisses the dialog from inside the header, in flow, so the header reserves the width it takes - * and a long title cannot run under it. `Dialog.CloseButton` stays the corner affordance, for - * dialogs that hold no card. + * and a long title cannot run under it. `Dialog.CloseButton` stays the corner affordance, for the + * `Profile`, which anchors its own. */ function HeaderCloseButton() { return ( @@ -102,8 +102,10 @@ const Header = React.forwardRef>(fun <> {/* First in the DOM, so it is the first tabbable element and takes the dialog's opening focus — the same reason `Dialog.CloseButton` is a part rather than a popup flag. - Not for an inline dialog, which nothing closes. */} - {isOverlayDialog(dialog) ? : null} + Not for an inline dialog, which nothing closes, and not for an alert dialog. */} + {/* An alert dialog interrupts to ask for a decision, and a corner X is a way out + without answering one. The cancel action in the footer is the way out. */} + {isOverlayDialog(dialog) && dialog?.role !== 'alertdialog' ? : null}
{children}
diff --git a/packages/ui/src/mosaic/components/dialog/alert-dialog.test.tsx b/packages/ui/src/mosaic/components/dialog/alert-dialog.test.tsx index 3783da325af..12f6e1dca9d 100644 --- a/packages/ui/src/mosaic/components/dialog/alert-dialog.test.tsx +++ b/packages/ui/src/mosaic/components/dialog/alert-dialog.test.tsx @@ -1,9 +1,9 @@ -import * as stylex from '@stylexjs/stylex'; import { act, cleanup, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; import { afterEach, describe, expect, it, vi } from 'vitest'; +import { Card } from '../card'; import type { DialogRootProps } from './dialog'; import { Dialog } from './dialog'; @@ -24,19 +24,23 @@ function Confirm({ onOpenChange, ...rest }: Partial = {}) { onOpenChange={onOpenChange} > - Discard changes? - This address has not been saved. - - Keep editing - - + + + Discard changes? + This address has not been saved. + + + Keep editing + + + ); } describe('role="alertdialog"', () => { - it('renders as an alertdialog, named and described by its parts', () => { + it('renders as an alertdialog, named and described by the card inside it', () => { render(); const popup = screen.getByRole('alertdialog', { name: 'Discard changes?' }); @@ -50,8 +54,12 @@ describe('role="alertdialog"', () => { role='alertdialog' > - Discard changes? - This address has not been saved. + + + Discard changes? + This address has not been saved. + + , ); @@ -66,26 +74,38 @@ describe('role="alertdialog"', () => { expect(document.querySelector('.cl-dialog-backdrop')).toBeInTheDocument(); expect(document.querySelector('.cl-dialog-viewport')).toBeInTheDocument(); expect(document.querySelector('.cl-dialog-popup')).toBeInTheDocument(); - expect(document.querySelector('.cl-dialog-actions')).toBeInTheDocument(); }); - it('is always the prompt size, and warns when asked for another', () => { + // The role decides the dismissal policy, and nothing else: an alert is a `Card` at the card + // size like every other dialog, and may be asked for any size and placement. + it('takes the size it is given, like any other dialog', () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + render(); + + expect(document.querySelector('.cl-dialog-popup')).toHaveAttribute('data-size', 'card'); + expect(warn).not.toHaveBeenCalled(); + warn.mockRestore(); + }); + + // A corner X answers the question by leaving, which is the one thing an alert must not offer. + it('renders no dismiss in the card header, unlike a plain dialog', () => { + const { unmount } = render(); + expect(screen.queryByRole('button', { name: 'Close' })).not.toBeInTheDocument(); + unmount(); + render( - - - Discard changes? - This address has not been saved. + + + + + Add email address + + , ); - expect(document.querySelector('.cl-dialog-popup')).toHaveAttribute('data-size', 'prompt'); - expect(warn).toHaveBeenCalledWith(expect.stringContaining('size="profile"')); - warn.mockRestore(); + expect(screen.getByRole('button', { name: 'Close' })).toBeInTheDocument(); }); it('opens from a trigger', async () => { @@ -94,8 +114,12 @@ describe('role="alertdialog"', () => { Delete - Delete this key? - Applications using it stop working. + + + Delete this key? + Applications using it stop working. + + , ); @@ -107,7 +131,9 @@ describe('role="alertdialog"', () => { expect(screen.getByRole('alertdialog')).toBeInTheDocument(); }); - it('opens focused on the cancel button, as the first element in the actions row', async () => { + // With no corner dismiss to take it, the opening focus lands on the first button in the footer + // — which is why the cancel is rendered first. + it('opens focused on the cancel button, as the first element in the footer', async () => { render(); // `FloatingFocusManager` moves focus asynchronously after mount, so this waits rather than @@ -148,11 +174,15 @@ describe('focus', () => { onOpenChange={setConfirmOpen} > - Discard changes? - This address has not been saved. - - Keep editing - + + + Discard changes? + This address has not been saved. + + + Keep editing + + @@ -212,11 +242,15 @@ describe('dismissal', () => { }} > - Discard changes? - This address has not been saved. - - Keep editing - + + + Discard changes? + This address has not been saved. + + + Keep editing + + ); @@ -239,7 +273,11 @@ describe('dev warnings', () => { role='alertdialog' > - Discard changes? + + + Discard changes? + + , ); @@ -247,7 +285,7 @@ describe('dev warnings', () => { await settle(); expect(warn).toHaveBeenCalledWith(expect.stringContaining('no description')); - expect(warn).toHaveBeenCalledWith(expect.stringContaining('')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('')); warn.mockRestore(); }); @@ -256,7 +294,11 @@ describe('dev warnings', () => { render( - Notifications + + + Notifications + + , ); @@ -277,7 +319,11 @@ describe('dev warnings', () => { role='alertdialog' > - This address has not been saved. + + + This address has not been saved. + + , ); @@ -285,7 +331,7 @@ describe('dev warnings', () => { await settle(); expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')); - expect(warn).toHaveBeenCalledWith(expect.stringContaining('')); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('')); warn.mockRestore(); }); @@ -300,90 +346,28 @@ describe('dev warnings', () => { }); }); -describe('Dialog.Actions', () => { - it('composes consumer xstyle onto the row', () => { - const caller = stylex.create({ actions: { marginBlockStart: '2rem' } }); - render( - - - Discard changes? - This address has not been saved. - - Keep editing - - - , - ); - - expect(screen.getByTestId('actions')).toHaveClass( - 'cl-dialog-actions', - stylex.props(caller.actions).className ?? '', - ); - }); - - it('merges the className a render source hands the row', () => { - render( - - - Discard changes? - This address has not been saved. - } - data-testid='actions' - > - Keep editing - - - , - ); - - expect(screen.getByTestId('actions')).toHaveClass('cl-dialog-actions', 'from-render'); - }); - - it('renders as another element through render', () => { - render( - - - Discard changes? - This address has not been saved. -