Conversation
|
Size Change: 0 B Total Size: 7.74 MB ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
commented
Mar 19, 2026
aduth
reviewed
Mar 19, 2026
ciampo
commented
Mar 25, 2026
ciampo
added a commit
that referenced
this pull request
Mar 31, 2026
Replace #XXXXX with the actual PR number #76438. Made-with: Cursor
|
Flaky tests detected in 846af7e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24138140325
|
ciampo
commented
Mar 31, 2026
Contributor
Author
ciampo
added a commit
that referenced
this pull request
Mar 31, 2026
Replace #XXXXX with the actual PR number #76438. Made-with: Cursor
aduth
reviewed
Mar 31, 2026
Contributor
Author
|
@WordPress/gutenberg-design cc'ing you in case you have any design feedback. You can run storybook on this PR's branch to take a close look, but I'm attaching a couple of screenshots for quick reference; click to expand
|
Replace `all: unset` with targeted property resets so the native browser focus outline is preserved on keyboard navigation. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
- Accessibility tests now verify the popup element has aria-labelledby and aria-describedby pointing at the Title/Description ids, not just that the ids exist. - Unstyled variant test compares against the default variant's class list instead of asserting an empty className string, making it resilient to Base UI adding runtime classes. Made-with: Cursor
- Use `toBeVisible()` instead of `toBeInTheDocument()` when asserting popover content is shown — stronger assertion that checks visibility. - Replace `waitFor` + `getBy*` with `findBy*` for cleaner async waits. - Prefer `findByRole` over `findByText` where a semantic role exists. - Store queried elements in variables to avoid redundant lookups. Made-with: Cursor
…trapping - Replace `Dialog.Popup.Props['initialFocus']` with `Popover.Popup.Props['initialFocus']` in the shared `useDeprioritizedInitialFocus` hook, removing the Dialog import. - Add JSDoc for the `modal` prop on `RootProps` explaining each value and the requirement to include `Popover.Close` for focus cycling. - Update `Popup` component JSDoc with the same guidance. Made-with: Cursor
The modal prop lives on Root, so the requirement for Popover.Close when focus trapping is active belongs in Root's JSDoc, not Popup's. Made-with: Cursor
Base UI's own JSDoc already documents the modal values and the Popover.Close requirement, so no need to duplicate it here. Made-with: Cursor
The Popover primitive hasn't shipped yet — move it out of the 0.10.0 released section into Unreleased. Made-with: Cursor
fillRule, clipRule, strokeLinejoin, and strokeMiterlimit were SVG editor export artifacts with no visual effect on the simple arrow paths. Made-with: Cursor
Made-with: Cursor
- Remove `margin: 0` from `.title` and `.description` CSS — now handled by the `Text` component after #76970. - Remove the empty `.title` CSS block and its references in title.tsx. - Explicitly destructure and forward the `render` prop in both Title and Description, making the consumer API intentional. - Move `ref` to the outer `Text` component, letting Base UI's `useRender` forward it through the composition chain. - Document the `<VisuallyHidden render={<Popover.Title />}>` pattern in the Title JSDoc. Made-with: Cursor
Replace `<Popover.Title><VisuallyHidden>text</VisuallyHidden></Popover.Title>`
with `<VisuallyHidden render={<Popover.Title />}>text</VisuallyHidden>` across
all stories. This preserves the `<h2>` semantics and ARIA wiring from
`_Popover.Title` while applying visually-hidden styles directly to the heading,
avoiding extra wrapper elements.
Made-with: Cursor
Remove the built-in `inline` prop from `Popover.Popup`. Consumers who need inline (non-portaled) rendering can achieve the same result by creating a local ref to a `<span style="display: contents">` and passing it as the `container` prop — this is now documented in the Inline story. This simplifies the component by removing a special code path and keeps the API surface small, since `container` already covers the use case. Made-with: Cursor
This was referenced Apr 8, 2026
ciampo
added a commit
that referenced
this pull request
Apr 8, 2026
Add a `container` prop to Dialog.Popup, AlertDialog.Popup, Tooltip.Popup, and Select.Popup, forwarding it to the underlying Base UI Portal. This lets consumers render overlays into a custom DOM node (e.g. for z-index stacking contexts or iframe scenarios). Popover.Popup already has this prop from #76438. Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 8, 2026
Add a `container` prop to Dialog.Popup, AlertDialog.Popup, Tooltip.Popup, and Select.Popup, forwarding it to the underlying Base UI Portal. This lets consumers render overlays into a custom DOM node (e.g. for z-index stacking contexts or iframe scenarios). Popover.Popup already has this prop from #76438. Made-with: Cursor
aduth
approved these changes
Apr 8, 2026
Comment on lines
+33
to
+34
| // Matches the popup's border-radius (--wpds-border-radius-md). | ||
| arrowPadding = 8, |
Member
There was a problem hiding this comment.
Tricky one to avoid hard-coding 🤔 I don't suppose there's an easy way to evaluate the actual token value in this context. Not a blocker, but may make future maintenance a bit harder, or if we add some theme configuration that affects radii.
Contributor
Author
There was a problem hiding this comment.
Yeah. More than the token value, the correct solution would be to resolve the value of the border radius at runtime (and watch for changes), but that feels like an overengineered solution. For now, I made the comment even more explicit. We can evaluate more involved solutions if the need arises
Member
There was a problem hiding this comment.
If we got #76604 working, we would be able to to at least get that default 8 value from a canonical source though!
ciampo
added a commit
that referenced
this pull request
Apr 9, 2026
Add a `container` prop to Dialog.Popup, AlertDialog.Popup, Tooltip.Popup, and Select.Popup, forwarding it to the underlying Base UI Portal. This lets consumers render overlays into a custom DOM node (e.g. for z-index stacking contexts or iframe scenarios). Popover.Popup already has this prop from #76438. Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 10, 2026
Add a `container` prop to Dialog.Popup, AlertDialog.Popup, Tooltip.Popup, and Select.Popup, forwarding it to the underlying Base UI Portal. This lets consumers render overlays into a custom DOM node (e.g. for z-index stacking contexts or iframe scenarios). Popover.Popup already has this prop from #76438. Made-with: Cursor
ciampo
added a commit
that referenced
this pull request
Apr 10, 2026
…77163) Add a `container` prop to Dialog.Popup, AlertDialog.Popup, Tooltip.Popup, and Select.Popup, forwarding it to the underlying Base UI Portal. This lets consumers render overlays into a custom DOM node (e.g. for z-index stacking contexts or iframe scenarios). Popover.Popup already has this prop from #76438. Made-with: Cursor --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: aduth <aduth@git.wordpress.org>
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
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.
What?
Add
Popovercomponent to@wordpress/ui, based on Base UI's Popover.Why?
We're building a new set of UI primitives — see #71196.
The new Popover is designed to eventually replace the monolithic
Popoverfrom@wordpress/components. It is architecturally cleaner (compound components, design tokens, CSS Modules) while covering the same use cases.How?
Root,Trigger,Popup,Arrow,Title,Description,Close.@wordpress/componentsPopover. Storybook examples (21 stories) demonstrate each capability.Implementation details
Sub-components
Popover.RootPopover.TriggeropenOnHover)Popover.PopupPopover.ArrowPopover.TitleTextwithheading-xlvariantPopover.Descriptionaria-describedby. UsesTextwithbody-mdvariantPopover.CloseKey features
side,align,sideOffset,alignOffsetonPopover.PopuparrowPaddingprop (default8) prevents arrow/corner overlapcollisionAvoidance,collisionBoundary,collisionPadding,stickyanchorprop (Element, VirtualElement, RefObject, callback)open/onOpenChange/defaultOpenonPopover.Rootmodal={true}(full modal) ormodal="trap-focus"(focus trap only). RequiresPopover.Closeinside the popup for focus cycling.backdropprop onPopover.Popupcontainerprop with a localdisplay: contentselement (documented in Inline story)containerprop for iframe scenariosopenOnHover,delay,closeDelayonPopover.Triggervariant="default"orvariant="unstyled"--available-height/--available-widthCSS variables--wp-ui-popover-z-indexCSS variableinitialFocus/finalFocus; close button deprioritized via shared hookPopover.Titleis always present and non-emptyDesign decisions
styleandclassNameare forwarded to the Positioner element (for z-index and CSS variable access). Other HTML attributes go to the inner Popup element. Documented inPopupPropsJSDoc; matches the Select component's pattern.Rootserves as the component overview for Storybook — noparameters.docs.descriptionneeded.Popover.Titlehas no built-in margin. Consumers add spacing viaStackor inline styles, making the component more composable and avoiding unintended layout from visually hidden titles.Storybook stories (21)
Default, NoArrow, Positioning (all side×align combos), WithCloseButton, Controlled, Modal, Unstyled, OverlayPlacement, Inline, CollisionAvoidance, CrossIframe, CrossIframe (SlotFill), WithCustomZIndex, Anchor (4 anchor types), ToolbarVariant, ViewportConstrainedSize, OnOpenChangeDetails, InitialFocus, TrapFocus, HoverTrigger, InfoTip.
Comparison with legacy Popover
anchor/anchorRef/getAnchorRectanchorprop (Element, VirtualElement, RefObject, callback)containerprop + external SlotFill (demonstrated in story)resize--available-height/--available-widthCSS variablesconstrainTabbingmodal="trap-focus"variant="toolbar"variant="unstyled"+ custom CSS (documented in story)focusOnMountinitialFocusproponClose/onFocusOutsideonOpenChangewitheventDetails.reasonexpandOnMobileopenOnHover(new capability)backdrop(new capability)Testing Instructions
npm run test:unit packages/ui/src/popover/test/index.test.tsxnpm run storybook→ Design System > Components > Popover.Screenshot
Follow-ups
Popoverwe can have infotips/hovercards (see UI Tooltip: Improve documentation to cover intended accessibility practices #76705).rgbacolor with a WPDS overlay/scrim token when one is introduced (tracked via inline TODO).renderprop and ref forwarding across all@wordpress/uicomponents. Currently three patterns coexist (explicit with fallback, implicit via rest, through Base UI). A follow-up should standardize on a single approach for consistency.