Repository navigation
fix(admin): move admin dialogs to Kumo's LayerDialog - #4495
khoinguyenpham04 wants to merge 50 commits into
Conversation
🦋 Changeset detectedLatest commit: 8528211 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-ai-search
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving the admin dialogs onto Kumo's LayerDialog is the right approach. It replaces hand-built dialog chrome with the design system's dialog and gives CapabilityConsentDialog and UninstallConfirmDialog a focus trap and Escape handling. One accessibility regression needs fixing: confirmation messages lose their association with the dialog's accessible description. Only one reviewer raised it; the other found nothing needing a fix, but its review did not mention description wiring. The other reviewer also could not check Kumo 2.15's LayerDialog props against type declarations, so it relied on the PR's passing typecheck.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
There was a problem hiding this comment.
Moving the admin dialogs onto Kumo's LayerDialog is the right approach. It replaces hand-built dialog chrome with the design system's dialog and adds focus trapping and Escape handling to the plugin permissions and uninstall dialogs. One accessibility regression from the previous review is still open at this head. The confirmation message in ConfirmDialog and in the inline confirmations (ContentList, ContentSettingsPanel, EntryLockNotice) is now a plain Text paragraph. It is no longer the dialog's accessible description, and no accessible-description assertions were added. Other dialogs in this PR already use LayerDialog.Description, so the fix is available. Neither reviewer found other issues. Both reviews were static and did not run tests, builds or lint, and Kumo's LayerDialog prop types could not be checked directly, so the passing typecheck is the only evidence there.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol. The change since the last review (1376 lines) was too large for a scoped follow-up, so this is a full review.
danielmlr
left a comment
danielmlr
left a comment
There was a problem hiding this comment.
Hi Noah, 5 items that are worth to look at:
- Just a heads-up that it's conflicting with #4308's branch, so the last six commits haven't had a CI run yet.
- The notes behind the new info buttons don't reach screen readers yet, since the tooltip isn't linked to its button. Some used to be visible text, like "Slugs cannot be changed after the field is created." A hidden span with the note and
aria-describedby, like the name span next to it, should fix that. A Popover withopenOnHoverwould work too. - The later commits go beyond the move and change what's inside the dialogs: section headings, notes moved behind info buttons, the avatar and the Delete row. The screenshots cover seven dialogs, so most of the redesigned ones haven't been seen yet. Before and after shots of the plugin permissions, uninstall, invite link, taxonomy, menu, widget and section dialogs would help, plus one form with info buttons in Arabic. Moving those commits into a follow-up PR would work just as well, and this one would stay the move.
- On phones, the form sheets (field, relation, redirect and others) can be swiped away, and what was typed is lost. The byline form's swipe guard on those dialogs feels like the safer default.
- On the emdashbot threads: ConfirmDialog's message is still the description, but without a Description slot the whole Body is, error text included.
LayerDialog.Descriptionwould keep it to the message.
emdashbot
Bot
added
review/needs-review
github-actions
Bot
added
review/awaiting-author
LayerDialog renders its own Close and Cancel labels, so the admin now supplies translated ones through KumoLocaleProvider. On phones LayerDialog is a full-width bottom sheet, which the max-width rule for centred dialogs would squeeze, so that rule now skips popups carrying data-swipe-direction. Its actions stay pinned below a body that scrolls on its own, so an error they raise can land out of sight at the end of a long form. DialogError now scrolls itself into view when a message appears.
ConfirmDialog and the confirmations written inline in the content list, the entry settings panel, the entry lock notice and the plugin draft preview now use LayerDialog: the title in a fixed header, the copy in a card, and Cancel and the action in a tray. Destructive confirmations that used role="alertdialog" use LayerDialog.Alert, and preventCloseWhilePending maps to dismissDisabled. ConfirmDialog drops titleClassName, descriptionClassName and compact, because LayerDialog fixes its own typography and spacing. Confirmations that closed through Dialog.Close now track their open state and close themselves when confirmed. The plugin draft preview refuses a swipe on a phone, as it already refused a press outside, so its changes can't be thrown away by accident.
Converts the widget area, menu, menu item, section, redirect, taxonomy, term, byline, byline field, media folder, domain, invite, allowed domain, plugin block, publication date and welcome dialogs. Each form lives in the body and submits through a form= button in the actions, so Enter and native validation still work. Dismissing while a save is pending is blocked where Cancel was disabled before. LayerDialog has room for one action, so Delete in the byline and media folder dialogs moves to the end of the content, and the invite link view offers Done and Copy invite link. The byline form refuses a phone swipe, as it already refused a press outside, so typed details survive. The 320px scheduling test now waits for the sheet to finish sliding up before it measures it.
…rDialog LayerDialog's dismiss button always closes its dialog, so the relation step's Back can no longer be a second button in the field dialog. The relation form now opens in its own dialog on top, whose dismiss button says Back, and returns to the field when it closes. RelationFormPanel now renders LayerDialog content and reports saves in flight, so RelationDialog can refuse to close under one. RelationDialog replaces the panel as it opens rather than as it closes: replacing the popup during its exit transition left it mounted, and building the form at mount froze defaults such as collection labels that load later.
Both were hand-built overlays without focus trapping or Escape. They now use LayerDialog, so focus stays inside, Escape cancels, and each is named by its visible title instead of "Capability consent" and "Uninstall confirmation".
Plugin admin pages ask for confirmation in the same dialog as the rest of the admin, and the confirmation now has a real dialog title.
… dialogs DialogSection gives grouped fields in a dialog one heading style, with a divider above every group but the first. HelpTooltip and HelpLabel put explanatory text behind an info button that stays out of the name of the field whose label holds it. A tap now pins the tooltip open: the hover a tap emulates used to close it straight away, so help buttons that open on press never worked on touch screens.
The relation form said a slug could not be generated from the role names as soon as the linking side was filled in, before the linked side had a name to generate one from.
The avatar picker filled the byline dialog with a full-width drop box. It is now a 64px circle with its Select, Replace, Edit asset and Remove buttons beside it.
Section headings use one style instead of uppercase eyebrows and small labels, selects fill their column, the relation slug follows the role names it is built from, and notes that only explain move behind info buttons. Errors, the reference conversion warning and the new relation slug's format hint stay visible.
Delete in the byline and media folder dialogs becomes a row at the end that says what deleting does. The byline field dialog's notes move behind info buttons, the redirect form's fields stack on phones, and the welcome dialog shows the logo above its title again, with Get Started in the tray.
The menu, section, taxonomy, invite, domain and widget dialogs, the plugin block dialog and the publication date dialog use the same field spacing, full-width selects and help buttons.
emdashbot
Bot
added
review/needs-rereview
github-actions
Bot
added
review/approved
… into noah/dialog-layer-dialog-migration-1ba0e6 # Conflicts: # packages/admin/src/components/ContentList.tsx # packages/admin/src/components/TaxonomyManager.tsx # packages/admin/src/components/comments/CommentInbox.tsx # packages/admin/src/router.tsx # packages/admin/tests/components/TaxonomyManager.test.tsx # packages/admin/tests/routes/byline-schema.test.tsx
The bulk "Delete Permanently?" confirmation shows its failure, so the comments page no longer also raises the bulk-action toast for a delete. Approving, marking as spam and trashing still report a failure in the toast, since no dialog is open for them.
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Moving the admin's confirmation and form dialogs to Kumo's LayerDialog is the right approach. The earlier accessible-description findings stay resolved: ConfirmDialog, the inline confirmations, and the Block Kit confirmation all use LayerDialog.Description, and regression tests cover them. One new issue remains. Relation limits can silently save a different number than the user typed. openai/gpt-6.1-sol raised it. anthropic/claude-opus-5-5 found nothing to fix and did not examine this validation path. This was a static review only; no tests, builds, or lint were run.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol. The change since the last review (2629 lines) was too large for a scoped follow-up, so this is a full review.
…g the save A dialog showed the server's "already used" refusal on its field and left it out of the banner. When the field was edited while the request ran, it no longer held the refused value, so the refusal showed nowhere and the save failed without a message. The menu, widget area, section, term, taxonomy, redirect, relation, allowed domain, field, byline field and invite dialogs now show the refusal on its field only if the field still held the sent value when the answer came back, and in the banner otherwise. The field and byline field dialogs also keep any failure in the banner when the form changed while the save ran; a change after the failure still clears it.
The relation dialog's "At most…" limit and the field dialog's length and item limits were read with parseInt, so 2.5 was saved as 2 and 1e3 as 1, without a message. The form's noValidate had also turned off the browser's step check that used to refuse 2.5 in the relation dialog. These limits must now be whole numbers, the value sent is the value the form checked, and the messages say "Enter a whole number of N or more." A field limit whose text isn't a number at all, such as "1e", reads as empty and was dropped; the field dialog now points at it with "Enter a number." instead.
…e form Kumo's LayerDialog makes its body the dialog's accessible description when there is no Description, so screen readers read out the whole form, such as every field type or every day of the calendar, each time one of these dialogs opens. The field, schedule, publication date, rename menu, custom link, menu item, widget area, folder, byline field and plugin block dialogs now say what they are for in one sentence under the title, as the other dialogs in the admin do.
The plugin permissions and uninstall dialogs only disabled their action while the request was pending, and both clicks of a double click can land before that renders, so a double click sent the install, update or uninstall twice. They now ignore a second press within the same moment, as ConfirmDialog does; that guard moves to `useConfirmPress` so the three dialogs share it. A double click also no longer accepts the extra permissions or public routes an install asks for before they are shown.
…t dialog While the delete runs, its confirmation disables its button, focus falls to the page, and Escape reaches the edit dialog behind it as well. The page already ignores the edit dialog's close while a delete is open; this test fails without that guard, which no test covered.
`useSingleFlight` marked a submit as running before calling it, and a submit that threw before returning a promise left that mark set, so every later press of the dialog's action was ignored. It now clears the mark and rethrows. No current caller throws this way; the helper's other caller pattern, `useConfirmedRequest`, already guards against it.
"Couldn't take over this entry" stayed up when the lock passed from the person the take-over was tried against to someone else, so it read as a failed attempt against the new holder. It now clears when the holder changes as well as when the lock's state does.
…nges dialogs Both dialogs wrote out the same swipe refusal and pointer guard that `guardUnsavedInput(true, …)` gives, so a later change to the guard would have missed them. Behavior is unchanged; the swipe tests cover both.
emdashbot
Bot
added
review/needs-rereview
The field, byline field and invite dialogs moved focus to the first invalid field whenever the server refused the slug or address, even when that field had been edited during the save and the refusal went to the banner instead. Focus then jumped away from the field being typed in, such as to a Label that had been cleared. They now move focus only when the field shows the refusal.
There was a problem hiding this comment.
Moving the admin's confirmation and form dialogs to Kumo's LayerDialog is the right approach, and keeping Dialog for the pickers and wide dialogs is a reasonable split. Both reviewers approved with no new findings. The earlier items are resolved at this head. Relation and field limits now reject fractional values instead of truncating them, and the value sent is the value that was checked. The confirmations, inline confirmations and Block Kit confirmation render their message through LayerDialog.Description, with tests covering the accessible description. This was a static review only; no tests, builds or lint were run.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol. The change since the last review (1083 lines) was too large for a scoped follow-up, so this is a full review.
The byline field and invite dialogs compared the value kept from a failed save with what was sent last. Pressing the action again replaces that a moment before the old failure clears, so for one render a value just sent was marked as already used and focus moved onto it, though the server had refused the earlier value. The dialogs now keep what was sent together with the failure, so a new submit can't change the comparison. The field dialog gets the same change, although its page clears the failure in time.
There was a problem hiding this comment.
The follow-up is the right change: snapshotting both the submitted value and the form at failure prevents an old refusal from being reassigned when another save starts, without changing the dialog architecture.
I reviewed every hunk since the previous review, traced the shared failure-snapshot helper and parent save handlers, and checked the updated focus and retry regression tests. The earlier integer-validation and accessible-description findings remain resolved; the author's responses are addressed. I found no new issues to fix in this follow-up.
Static review only; no tests, builds, or lint were run.
Follow-up review by openai/gpt-6.1-sol, scoped to the changes since the last review.
emdashbot
Bot
added
review/needs-rereview
github-actions
Bot
added
review/approved
What does this PR do?
Moves the admin's dialogs to Kumo's
LayerDialog. Each dialog keeps its title in a fixed header, scrolls its content in a card, and puts Cancel and the main action in a tray along the bottom. On phones, dialogs open as bottom sheets.Inside the dialogs, groups of fields share one section heading style, selects fill their column, and paired fields stack on phones. Explanations that sat under fields are behind an info button beside the label. Errors, warnings about what an action will do, and format rules the dialog does not check stay visible. Every info button in the admin, inside dialogs or not, now opens on a tap as well as on hover and keyboard focus.
Stacked on #4308:
LayerDialogfirst shipped in Kumo 2.14, so this lands after the Kumo 2.15 upgrade.Converted to
LayerDialogConfirmDialog(35 confirmations), and the confirmations written inline in the content list, the entry settings panel (discard, publish, move to trash), the entry lock notice and the plugin draft preview.@emdash-cms/blocks.Kept on
Dialog: the media picker, media details, media upload, bulk tagging, content picker and section picker. They need more thanLayerDialog's 768px, several footer buttons, or a search field pinned above a scrolling list. The calendar and user side panels are unchanged.Behavior that comes with the design
LayerDialog.Description, Kumo describes a dialog by its whole body, so screen readers would read checkbox labels and error text as part of the message. Form dialogs get a one-sentence description for the same reason, so a screen reader doesn't read out every field type or calendar day each time one opens.LayerDialogholds one action, so Delete in the byline and media folder dialogs moves to a row at the end of the dialog that says what deleting does. The invite link view offers Done and Copy invite link.Error states and interactions
Every dialog now handles mistakes, slow requests and stray presses the same way:
aria-invalidplus its message), focuses the first, and sends nothing. Messages appear on the first submit and then follow the input. The checks mirror the server's schemas, so most mistakes never reach it, and a rule stored data may predate applies to new items only. A new select field also needs an option, and a new blocks field a block type, since editors can't fill in either without one. Length, item and relation limits must be whole numbers (2.5used to save as2), and a field limit whose text isn't a number, such as1e, is pointed at instead of being dropped.CONFLICT), a reserved field slug, a domain the check rejects, an existing user. When that field was edited while the request ran, the refusal shows in the error box instead, since the field no longer holds the refused value. Other failures show in the dialog's error box and are no longer also toasted, apart from a failed save of unsaved edits before scheduling or changing the publication date.accessories-1(the slug sent) instead of the server silently saving a different one; a MIME type typed but not added is saved with the field; menu links follow the API's own href rule, somailto:,tel:and#topwork; the plugin block URL takes any absolute URI exceptjavascript:,data:andvbscript:(Blueskyat://posts work); a plugin install asking for more permissions no longer reads as a failure; deleting a redirect that is already gone counts as deleted.Shared pieces live in
lib/dialog-form.ts:revealErrorsandfocusFirstInvalid,guardUnsavedInputforLayerDialog.Root,useConfirmedRequestfor confirmations that should stay open until their request lands,useSingleFlightfor submits,useConfirmPressfor a confirm action a double press should run once (ConfirmDialogand the plugin permissions and uninstall dialogs share it), anduseTakenValueanduseValueAtFailurefor putting a refusal on its field only while the field holds the refused value.ConfirmDialogalways locks while pending (itspreventCloseWhilePendingprop is gone), ignores a second press within half a second, and hides an error that predates its opening.DialogErrorputs focus back in the dialog when the control that started the request was disabled.Supporting changes
DialogSectiongives grouped fields one heading style, with an optional description, help button and section action.HelpTooltipno longer has a hover-only mode (openOnPressis gone), soFieldHelpLabel,HelpLabelandDialogSectionall open on a tap. The content editor's content language and byline help and the media details filename, alt text and focal point help, which were hover-only, now use it; the media details buttons are the standard info button, slightly smaller than their old bare icons.HelpTooltipandHelpLabelput explanatory text behind an info button. Base UI doesn't tie a tooltip to its trigger, so the note is repeated in a hidden element that describes the button.FieldHelpLabelnow usesHelpTooltip. Its tap-to-open mode used to open and close on the same tap, because the tooltip closed again when the hover that a tap emulates ended.KumoTranslationsProvidergivesLayerDialog's built-in Close and Cancel labels the admin's translations.LayerDialog's full-width sheets.DialogErrorscrolls itself into view, because the actions that raise an error stay pinned below a body that scrolls on its own.New regression tests cover these behaviors, and each fails without its fix:
ConfirmDialog.test.tsx,confirmation-failures.test.tsxon the real router, and the per-dialog files). For the review fixes, I reverted each fix and watched its test fail.Test updates replace removed × buttons with Cancel, open tooltips to check moved text and find them by the tooltip role (their text also matches the button's hidden description), use exact label matches where an info button's name also contains the label, and use DOM clicks where Base UI's inert overlay intercepts Playwright clicks on unstyled test pages.
Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI assistance disclosure
Screenshots / test output
Before is #4308 (Kumo 2.15 with the old dialogs); after is this branch. Same viewport and content for each pair, light theme.
The other redesigned dialogs.
Error states. Before is this branch before the error-state commits (
67e762fb0), so each pair shows only the error handling.Help on a phone. Before is this branch without the tap fix; after is after a tap, at 390px.
Help on a phone in Arabic
Error states on a phone (390px) and in Arabic
Phone (390px)
Arabic
Test output:
SeoPanelandBylineFiltertap tests fail with the old hover-onlyFieldHelpLabel.pnpm build,pnpm typecheckandpnpm lint:quickpass, andpnpm lint:jsonreports 0 diagnostics.