feat(dialog): add layer variant#603
Conversation
cbf3db3 to
bdf5d49
Compare
commit: |
Docs PreviewCommit: |
bdf5d49 to
54b5a5c
Compare
|
/bonk review |
There was a problem hiding this comment.
This is a clean, well-structured approach to the layer dialog treatment — a nice alternative to a standalone top-level component. Tests are thorough, docs are comprehensive, and the changeset looks good.
A few items to address before merging:
-
Missing main package exports — The new
DialogBody,DialogFooter,DialogSeparatorsubcomponents (and their types) are exported fromcomponents/dialog/index.tsbut are not re-exported from the main barrel filepackages/kumo/src/index.ts. Consumers won't be able to tree-shake import them directly (e.g.,import { DialogBody } from "@cloudflare/kumo"). Please add them to the mainsrc/index.tsDialog export block. -
Minor padding redundancy —
DialogBodyappliesp-6 pr-6for the layer variant. Sincep-6already setspr-6, the explicitpr-6utility is redundant. -
Visual regression noise — The report shows unexpected changes across several Button and Select screenshots. Since this PR only touches Dialog, those diffs look like baseline flakiness. Worth a quick sanity check, but not a blocker.
-
LayerCard structured-layer detection — Because
Dialog.BodyandDialog.Footerare wrapper components (functions that returnLayerCard.Primary/LayerCard.Secondary), the parentLayerCardinDialogContentwon't detect them viahasLayerCardSections. The explicitclassNameon the wrapper compensates, but this creates a subtle coupling withLayerCardinternals. Not blocking, but something to keep in mind ifLayerCardlogic evolves.
Once the main package exports are added, this should be good to land.
|
|
||
| return ( | ||
| <LayerCard.Primary | ||
| className={cn(variant === "layer" && "p-6 pr-6", className)} |
There was a problem hiding this comment.
p-6 already includes pr-6, so the explicit pr-6 is redundant.
| className={cn(variant === "layer" && "p-6 pr-6", className)} | |
| className={cn(variant === "layer" && "p-6", className)} |
| type DialogBodyProps, | ||
| type DialogFooterProps, | ||
| type DialogSeparatorProps, | ||
| } from "./dialog"; |
There was a problem hiding this comment.
These exports look correct, but don't forget to also re-export the new components and types from the main package barrel file (packages/kumo/src/index.ts) so consumers can import them directly from @cloudflare/kumo.
|
Review submitted for PR #603. Here's a summary of what I found: Blocking issue:
Minor but worth fixing:
Non-blocking observations:
What looks good:
Once the missing main index.ts exports are added, this should be good to land. |
7fea53e to
30f9039
Compare
|
I reviewed the Dialog usages scraped from Stratus with a focus on backwards compatibility for existing className/style extension patterns. I did not find a breaking change for existing/default Dialog usage. Compatibility confirmed:
One difference to document, but not a breaking change: for the new |
4967f27 to
1ebccd8
Compare































































Summary
This PR provides an alternative to #570 by bringing the Layer Dialog treatment into the existing
DialogAPI instead of introducing a new top-levelLayerDialogcomponent.variant="layer"toDialog, with the existing behavior preserved as the defaultvariant="base"Dialog.Body,Dialog.Footer, andDialog.SeparatorDialog.TitleandDialog.Descriptionbacked by the Base UI dialog primitives for accessibility semanticsValidation
pnpm --filter @cloudflare/kumo lint— passes with existing warningspnpm --filter @cloudflare/kumo typecheck— passescd packages/kumo && pnpm exec vitest run --project=unit src/components/dialog/dialog.test.tsx— passespnpm --filter @cloudflare/kumo codegen:registry— passes; generated outputs are ignored/untrackedNote:
pnpm --filter @cloudflare/kumo-docs-astro typecheckcurrently fails on unrelated existing docs/demo type errors across SkeletonLine, Switch, Table, Toolbar, Tooltip, Chart, SankeyChart, and TableOfContents.