AnchoredOverlay: Add Popover API, modifications to CSS anchor positioning#7677
AnchoredOverlay: Add Popover API, modifications to CSS anchor positioning#7677
Conversation
🦋 Changeset detectedLatest commit: a39e179 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
staff
|
TylerJDev
force-pushed
the
tylerjdev/add-popover-support-css-anchor-positioning
branch
from
b2ee25e to
b55ac54
Compare
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
TylerJDev
changed the title
TylerJDev
force-pushed
the
tylerjdev/add-popover-support-css-anchor-positioning
branch
from
69db3ec to
1c6dadf
Compare
TylerJDev
added
the
update snapshots
github-actions
bot
removed
the
update snapshots
There was a problem hiding this comment.
Pull request overview
This PR introduces Popover API support to AnchoredOverlay (behind the primer_react_css_anchor_positioning feature flag) and updates the CSS anchor positioning approach, including adding an opt-out for rendering overlays in a Portal.
Changes:
- Add
disablePortalsupport toOverlayto allow rendering without a Portal when desired. - Update
AnchoredOverlayto integrate Popover API (popover="manual",showPopover()), and to set anchor/position anchor styles via JS. - Extend unit/e2e coverage and Storybook examples (including a “Multiple Overlays” scenario) and add a changeset for a minor release.
Reviewed changes
Copilot reviewed 8 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Overlay/Overlay.tsx | Adds disablePortal prop and uses it to bypass Portal rendering. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.tsx | Adds Popover API wiring + JS-managed anchor-name / position-anchor setup. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.test.tsx | Updates tests for new portal behavior and feature-flag-specific behavior. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.module.css | Removes wrapper/anchor classes; adjusts anchored overlay styles for popover behavior. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.features.stories.tsx | Adds “Multiple Overlays” story to validate multiple instances. |
| packages/react/src/ActionMenu/ActionMenu.test.tsx | Removes assertions tied to the removed .Anchor CSS class. |
| e2e/components/AnchoredOverlay.test.ts | Adds multi-overlay screenshot coverage. |
| .playwright/snapshots/** | Adds new snapshots for the “Multiple Overlays” story. |
| .changeset/soft-pianos-carry.md | Declares a minor release for the new AnchoredOverlay behavior behind the flag. |
| renderAnchor={props => <Button {...props}>Anchor Button</Button>} | ||
| onPositionChange={onPositionChange} | ||
| className={className} | ||
| {...overlayProps} |
There was a problem hiding this comment.
AnchoredOverlayTestComponent is spreading the overlayProps test setting directly onto <AnchoredOverlay> ({...overlayProps}), which passes disablePortal as a top-level prop instead of as overlayProps={...}. This should be passed via the overlayProps prop (e.g. overlayProps={overlayProps}), otherwise TS should reject it and the test won't be exercising the intended behavior.
| {...overlayProps} | |
| overlayProps={overlayProps} |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/16986 |
|
Integration test results from github/github-ui:
|
liuliu-dev
left a comment
liuliu-dev
left a comment
There was a problem hiding this comment.
Looks great! ✨
Just a few questions to make sure I understand it correctly.
| onClick: onAnchorClick, | ||
| onKeyDown: onAnchorKeyDown, | ||
| ...(cssAnchorPositioning ? {className: classes.Anchor} : {}), | ||
| ...(cssAnchorPositioning ? {'data-anchor': '', popoverTarget: popoverId} : {}), |
There was a problem hiding this comment.
I couldn't find data-anchor used anywhere in the CSS, is this for future styles?
| // Track the overlay element so we can re-run the effect when it changes. | ||
| // This is necessary when using a Portal, as React re-renders can replace | ||
| // the DOM node, causing the popover to lose its :popover-open state. | ||
| const overlayElement = overlayRef.current | ||
|
|
||
| useLayoutEffect(() => { | ||
| // Read ref inside effect to get the value after child refs are attached | ||
| const currentOverlay = overlayRef.current | ||
|
|
||
| if (!cssAnchorPositioning || !open || !currentOverlay) return | ||
| currentOverlay.style.setProperty('position-anchor', `--anchored-overlay-anchor-${id}`) | ||
| try { | ||
| if (!currentOverlay.matches(':popover-open')) { | ||
| currentOverlay.showPopover() | ||
| } | ||
| } catch { | ||
| // Ignore if popover is already showing or not supported | ||
| } | ||
| }, [cssAnchorPositioning, open, overlayElement, id, overlayRef]) |
There was a problem hiding this comment.
Curious about this part. Is this because when a Portal remounts, it creates a new Overlay element and :popover-open is lost on this new element?
Prerequisite to https://github.com/github/primer/issues/6448, part of https://github.com/github/primer/issues/6431
Utilizes the Popover API to power our CSS anchor positioning in
AnchoredOverlay. Both the popover support and CSS anchor positioning are behind theprimer_react_css_anchor_positioningfeature flag.Changelog
New
AnchoredOverlayChanged
Rollout strategy
Testing & Reviewing
Merge checklist