api change proposal for dialog component - #5228
Conversation
There was a problem hiding this comment.
Pull request overview
This PR proposes a new compound-component API for the /next Dialog, refactoring Dialog into a context provider and moving native <dialog> rendering and dialog-specific props to Dialog.Popup, while adding Dialog.Trigger and Dialog.Close helpers to manage open/close behavior through context.
Changes:
- Made
Dialoga context provider with optionalopen/onOpenChangefor controlled or uncontrolled usage. - Introduced
Dialog.Popup(native<dialog>+ scrim/backdrop handling), plusDialog.TriggerandDialog.ClosewithasChildsupport viaSlot. - Updated the primary Storybook introduction story to demonstrate the new API.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/eds-core-react/src/components/next/Dialog/Dialog.types.ts | Adjusts public types to support the new compound API and moves dialog element props to DialogPopupProps. |
| packages/eds-core-react/src/components/next/Dialog/Dialog.tsx | Refactors implementation into a provider + Dialog.Popup renderer, and adds trigger/close compound components. |
| packages/eds-core-react/src/components/next/Dialog/Dialog.stories.tsx | Updates the introduction story to use Dialog.Trigger + Dialog.Popup + Dialog.Close. |
| /** | ||
| * Controlled open state. When true, the dialog is shown via the native | ||
| * `HTMLDialogElement.showModal()` (modal mode: focus trap + inert background). | ||
| */ | ||
| open: boolean | ||
| open?: boolean | ||
| /** | ||
| * Called when the dialog requests to close — via Escape key, backdrop click, | ||
| * or the header close button. Update your `open` state in response; the | ||
| * component is fully controlled. | ||
| */ | ||
| onOpenChange: (open: boolean) => void | ||
| onOpenChange?: (open: boolean) => void | ||
| children: ReactNode |
| const dialogRef = useRef<HTMLDialogElement | null>(null) | ||
| // Suppresses the onOpenChange call fired by the native `close` event when | ||
| // we close the dialog in response to a consumer flipping `open` to false. | ||
| // Without this, the consumer's setter would be called once externally and | ||
| // once from handleClose — see review thread on PR #4956. | ||
| const expectedCloseRef = useRef(false) | ||
| // Records whether the mousedown that started a click landed on the dialog | ||
| // element itself. Click fires on the common ancestor of mousedown/up, so a | ||
| // text-selection drag that overshoots into the backdrop would otherwise be | ||
| // mistaken for a backdrop click and close the dialog. | ||
| const mouseDownOnDialogRef = useRef(false) | ||
| const [titleId, setTitleId] = useState<string | undefined>(undefined) | ||
| const [open, setOpen] = useState(openProp ?? false) | ||
|
|
||
| useEffect(() => { | ||
| const dialog = dialogRef.current | ||
| if (!dialog) return | ||
| if (open && !dialog.open) { | ||
| dialog.showModal() | ||
| } else if (!open && dialog.open) { | ||
| expectedCloseRef.current = true | ||
| dialog.close() | ||
| } | ||
| }, [open]) | ||
|
|
||
| const setRef = useCallback( | ||
| (node: HTMLDialogElement | null) => { | ||
| dialogRef.current = node | ||
| if (typeof ref === 'function') ref(node) | ||
| else if (ref) ref.current = node | ||
| }, | ||
| [ref], | ||
| ) | ||
|
|
||
| const handleClose = () => { | ||
| if (expectedCloseRef.current) { | ||
| expectedCloseRef.current = false | ||
| return | ||
| } | ||
| onOpenChange?.(false) | ||
| } | ||
|
|
||
| const handleMouseDown = (event: MouseEvent<HTMLDialogElement>) => { | ||
| mouseDownOnDialogRef.current = event.target === dialogRef.current | ||
| } | ||
|
|
||
| // Native <dialog> reports the dialog itself as the click target when the | ||
| // backdrop is clicked; children dispatch from their own elements. The | ||
| // additional mousedown check guards against drag-out from a selection. | ||
| const handleClick = (event: MouseEvent<HTMLDialogElement>) => { | ||
| const wasOnBackdrop = | ||
| event.target === dialogRef.current && mouseDownOnDialogRef.current | ||
| mouseDownOnDialogRef.current = false | ||
| if (wasOnBackdrop) dialogRef.current?.close() | ||
| } | ||
| const onOpenChange = onOpenChangeProp ?? setOpen |
| <dialog | ||
| ref={setRef} | ||
| className={['eds-dialog', className].filter(Boolean).join(' ')} | ||
| data-scrim={scrim || undefined} | ||
| aria-labelledby={resolvedAriaLabelledBy} | ||
| aria-label={ariaLabel} | ||
| onClose={handleClose} | ||
| onMouseDown={handleMouseDown} | ||
| onClick={handleClick} | ||
| {...rest} | ||
| > |
| <Dialog> | ||
| <Dialog.Trigger>Open dialog</Dialog.Trigger> | ||
| <Dialog.Popup> | ||
| <Dialog.Header> | ||
| <Dialog.Title>Dialog title</Dialog.Title> | ||
| </Dialog.Header> | ||
| <Dialog.Content> | ||
| This is a short description of the action the user is about to take. | ||
| </Dialog.Content> | ||
| <Dialog.Actions> | ||
| <Dialog.Close>Close</Dialog.Close> | ||
| <Button onClick={() => {}}>Confirm</Button> | ||
| </Dialog.Actions> | ||
| </Dialog.Popup> | ||
| </Dialog> |
| if (typeof ref === 'function') ref(node) | ||
| else if (ref) ref.current = node | ||
| }, | ||
| [ref, ctx], |
|
@claude review |
|
Claude finished @pomfrida's task in 7m 40s —— View job Review: Dialog compound API proposalI like the direction — a provider + I ran the suite and some scratch tests locally to verify each claim below. 🔴 Blocking1. Controlled mode no longer works at all const [open, setOpen] = useState(openProp ?? false)
const onOpenChange = onOpenChangeProp ?? setOpen
I verified with two scratch tests, both fail on this branch: Uncontrolled ( The usual fix is the controllable-state pattern — keep internal state always, let the prop win when defined, and always call the consumer callback: const isControlled = openProp !== undefined
const [uncontrolledOpen, setUncontrolledOpen] = useState(defaultOpen ?? false)
const open = isControlled ? openProp : uncontrolledOpen
const onOpenChange = useCallback((next: boolean) => {
if (!isControlled) setUncontrolledOpen(next)
onOpenChangeProp?.(next)
}, [isControlled, onOpenChangeProp])This also fixes the "notify-only" case (uncontrolled dialog + 2. The test suite is not updated — CI is red
and running jest directly: 18 of 25 tests fail (everything that queries 3. Five of six stories still use the old API Only 4. The Code Connect 🟠 Correctness & API5. A consumer passing 6. Both bypass <a href="#x" class="dialog-button">Open link</a>No return <Button asChild={asChild} {...sharedProps}>{children}</Button>If the intent is genuinely "unstyled trigger", it needs a different props type and an explicit doc note — but the styled version seems more useful here. Fix this → 7.
8. Two different close paths
9. One
10. Root no longer forwards a ref to the 🟡 Minor
SummaryThe API is worth pursuing — my main asks before merge are: fix controlled mode, port the tests and the remaining stories, fix the Verified locally: |
This PR proposes a change to the
DialogAPI.The
Dialogcomponent becomes a pure context provider, while dialog-specific props are moved to a newDialog.Popupcomponent. It also introducesDialog.TriggerandDialog.Close, both of which supportasChildand integrate with the internal dialog context to control the open state.With this API, the dialog can manage its own state internally, removing the need for consumers to control it externally.
This pattern is even more beneficial for components such as
PopoverandMenu. In addition to managing their own state internally, they can also manage the ref used to anchor the popup.Popover.Triggercan own this logic and, throughasChild, attach the required ref to any element without exposing that implementation detail to consumers.