feat(popover): export Popover as a public compound component - #1936
Merged
Conversation
misama-ct
added
the
WIP
🦋 Changeset detectedLatest commit: b18c451 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Bundle Size ReportLast updated: 2026-08-25 06:14:39 UTC
Baseline source: comment-chain |
misama-ct
marked this pull request as ready for review
August 20, 2026 11:51
|
Tip All tests passed and all changes approved!🟢 UI Tests: 1226 tests unchanged |
Same audit, the surfaces outside the component directory. Every item below was checked against a source line, and the ones that turned out to be already correct are left alone. openspec/specs/nimbus-popover: - The exit animation "SHALL use the same duration as the enter animation". The recipe uses `fast` to enter and `faster` to exit — deliberately shorter, copied from Menu — so the spec described an animation we never shipped. - The isNonModal scenario required that focus "SHALL NOT be contained". The opposite is true and is now asserted by two stories; rewritten around inert and the scroll lock, which is what modality actually changes here. - The accessible-name scenario assumed a visible heading names the dialog. RAC falls back to the trigger's name instead. - The maxHeight carve-out named only `maxH`; `maxBlockSize` is equally accepted and equally unable to cap the surface. openspec/specs/nimbus-localized-field: the info-box scenario still specified the surface treatment its own recipe used to carry (bg neutral.1, boxShadow 6, focusRing outside). After the migration onto the Popover compound, infoDialog carries only sizing and scrolling, the surface comes from the compound's content slot, and the only focus ring is the hint trigger's. Archived 2026-08-20-popover-compound-export. These are historical records, so only present-tense claims about the shipped API are corrected and the narrative framing is left intact: - design.md decided LocalizedField would drop to bare React Aria and gain an `infoPopover` slot, concluding the compound ships with zero internal consumers. It shipped with one: LocalizedField renders the compound, no `infoPopover` slot exists, and focusRing was removed rather than relocated. Recorded as a Superseded note in the file's existing style rather than by rewriting the decision — the ComboBox half still holds, and the paragraph above it that reads the old recipe as evidence is correctly in past tense. - proposal.md's impact table claimed the same three LocalizedField changes. - tasks.md 5.8 recorded asserting `aria-haspopup="dialog"` on the trigger; the story asserts its absence, and the change's own design.md argues for that. - tasks.md 3.1 described Root splitting recipe variants and extracting style props (it does neither, and the recipe declares no variants), and 2.2 named a recipe-props layer that popover.types.ts does not have. - The delta's portal scenario predated `UNSTABLE_portalContainer` landing on Root, and its offset/crossOffset scenarios were unqualified as to which part they are set on, which now reads as the wrong part. docs/file-type-guidelines/compound-components.md illustrated flexible child composition with `Popover.Title`, `Popover.Description` and `Popover.Actions`, none of which exist, and no trigger. Retargeted to Dialog, which really has those parts (verified against dialog.tsx: Header/Title/Body/Footer/ CloseTrigger), and added a Popover example showing the opposite case, where the consumer's own markup fills that role. Line 26 of this file had already been corrected to three parts; the sample 440 lines down had not. Untouched deliberately: the earlier serena/openspec metadata, the released CHANGELOG, and archived prose that is already framed in the past tense. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
misama-ct
force-pushed
the
FEC-1167-popover-compound-export
branch
from
August 21, 2026 11:22
8fc7cbb to
4948673
Compare
…y naming requirements
Out of scope for this branch, which adds the Popover component. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The type-level blocks had grown into essays duplicating popover.dev.mdx and the nimbus-popover spec. Keep the actionable half — what a part rejects and where to set it instead — and link the docs site for the rest. Per-property JSDoc is untouched, so the docgen output is unchanged. Comments only; the type surface is identical. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Popover.Content renders a React Aria Dialog, which publishes a ButtonContext with a close slot, so `<Button slot="close">` works inside it — the same way it already does in Dialog and Drawer. The docs only showed the function-child render prop, so the conventional form looked unavailable. Lead with slot="close" and keep the render prop for the case it is actually needed: an action that has to run before the popover dismisses. Add a CloseViaSlot story so the behaviour is covered rather than just claimed. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Where configuration goes" restated the API design rather than showing usage: its prop table duplicated the generated <PropsTable>, its example duplicated the one in "Positioning" directly above it, and no other component documents configuration this way — Root taking the configuration is the house pattern. The exclusion rationale it carried is already in the nimbus-popover spec. "Styling" was one of only two such sections across 88 dev.mdx files. Usage examples is now feature sections with live examples throughout, matching the peer components and the template. FEC-1167 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
misama-ct
removed
the
WIP
ByronDWall
approved these changes
Aug 24, 2026
ByronDWall
left a comment
ByronDWall
left a comment
Contributor
There was a problem hiding this comment.
One very small nit on the docs. The component behaves as expected in storybook, code looks good. approved, thanks for implementing this!
Comment on lines
+89
to
+98
| `isNonModal` does not lift that containment. What it changes is the page behind | ||
| the popover: by default React Aria marks the rest of the page `inert` and locks | ||
| page scroll, and `isNonModal` leaves both alone, so assistive technologies can | ||
| reach the surrounding content. Focus still cycles inside the dialog and an | ||
| outside press still dismisses it — containment comes from the dialog | ||
| `Popover.Content` renders, not from modality. React Aria's own guidance is that | ||
| most popovers should not use it, because screen readers can announce the | ||
| surrounding page in ways that make the popover harder to follow. It exists for | ||
| components like combobox, where the trigger must keep focus while the overlay is | ||
| open. |
Contributor
There was a problem hiding this comment.
nit: this paragraph seems like a better fit for the 'implementation' section?
The `isNonModal` paragraph in the Accessibility tab documented prop mechanics (`inert`, page scroll, why the prop exists) rather than accessibility behavior, and `popover.dev.mdx` already carried a condensed copy of the same claim in its Component requirements section. Keep the a11y-facing consequence in `popover.a11y.mdx` — containment is not lifted, and most popovers should not use the prop — and move the mechanics into the Implementation tab's Dismissal behavior section, alongside `isKeyboardDismissDisabled` and `shouldCloseOnInteractOutside` and matching the spec's "dismissal and modality" grouping. The condensed duplicate becomes a Modality bullet pointing there.
github-actions
Bot
added
the
bundle-sizes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Popoveralready existed, but only as an internal single-element wrapper — itsown JSDoc said "this component is only used internally" — sitting behind
Combobox's dropdown and LocalizedField's hint box. This promotes it to a public
compound component (
Popover.Root,Popover.Trigger,Popover.Content) andmoves both internal consumers off the old shape.
Ships as Beta. Two
minorchangesets: the new component, and a LocalizedFieldaccessibility fix that fell out of the migration.
Motivation
There was no anchored-overlay primitive between
Tooltip(plain hints) andDialog(flows that should block the page). Filter panels, short edit forms andcontext-sensitive actions had to be hand-rolled from React Aria's
DialogTrigger+Popover+Dialog— which is exactly what LocalizedFieldwas doing, and it shipped an unnamed dialog as a result.
Why not split
The recipe registration moves from
recipestoslot-recipes:nimbusPopoverbecomes a slot recipe the moment the compound lands. Both existing consumers
read the old plain recipe, so they change meaning in the same commit and have to
move with it. The docs set, spec rewrite and consumer migrations are all
downstream of that one switch.
Map
popover/components/popover.{root,trigger,content,context}.tsxpopover.recipe.tsx,popover.slots.tsx,theme/{recipes,slot-recipes}/index.tspopover.types.ts,components/index.tspopover.{mdx,dev.mdx,a11y.mdx,guidelines.mdx}popover.stories.tsx,popover.docs.spec.tsxcombobox/**,localized-field/**Popoverdirectly; LocalizedField adopts the public compoundopenspec/specs/nimbus-popover/,openspec/changes/archive/**How to review
openspec/changes/archive/2026-08-20-popover-compound-export/design.md— theAPI decisions, so the rest reads as consequences.
popover.types.ts, then the fourcomponents/*.tsx— contract, thenimplementation.
popover.stories.tsx— the behaviour, in play-function form.could regress.
The docs and openspec files are prose — skim or skip.
Test plan
pnpm test:dev packages/nimbus/src/components/popover/— 27 pass (19stories + 8 consumer tests)
pnpm --filter @commercetools/nimbus typecheck:devcleanbuild-and-testandchromatic— still running at time of writingHintDialogsnapshot changes on purpose: one shadow instead of two stacked, no border
or background tint of its own, fades in and out like other popovers, and
no outline around itself when open (matching
DialogandDrawer).Risk
Medium. The new component is additive behind a new export, but two shipped
components changed rendering path. Combobox is the one to watch: it no longer
routes through the shared recipe, so
zIndex: 1is now declared on its ownpopoverslot to keep stacking order unchanged.Rollback is a plain revert — no migration, no infrastructure, no persisted
state. Popover is unreleased, so only the Combobox and LocalizedField changes
have consumer-visible surface.
FEC-1167