Conversation
🦋 Changeset detectedLatest commit: 414cc91 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 |
|
ab4489b to
e949fce
Compare
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/18562 |
There was a problem hiding this comment.
Pull request overview
Adds Popover API support to Overlay to better interoperate with AnchoredOverlay when CSS anchor positioning / top-layer popovers are in use.
Changes:
- Adds a
popoverprop toOverlayand attempts to auto-open viashowPopover(). - Adjusts
OverlayCSS to reset UA popover styles. - Adds a nested-overlay dev story and VRT coverage for the nested scenario (with and without the feature flag).
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Overlay/Overlay.tsx | Introduces popover prop and showPopover() behavior in Overlay. |
| packages/react/src/Overlay/Overlay.module.css | Adds popover-specific CSS reset rules for Overlay. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.module.css | Adds/clarifies comment in anchored overlay positioning CSS. |
| packages/react/src/AnchoredOverlay/AnchoredOverlay.dev.stories.tsx | Adds a dev story demonstrating a nested Overlay inside an AnchoredOverlay. |
| e2e/components/AnchoredOverlay.test.ts | Adds a VRT scenario that opens a nested overlay (runs in both flag modes). |
| .changeset/pretty-masks-kick.md | Declares a minor release changeset for the new Overlay popover API support. |
Copilot's findings
Comments suppressed due to low confidence (1)
packages/react/src/Overlay/Overlay.tsx:246
showPopover()is triggered wheneverpopoveris set, even ifvisibility === 'hidden', and there’s no correspondinghidePopover()whenvisibilitybecomes hidden or whenpopoveris unset. This can leave the element in:popover-openstate while the component is “closed” viavisibility, which may impact focus/interaction. Align this effect with the existing visibility semantics (skip when hidden, and/or hide the popover in cleanup when closing).
useLayoutEffect(() => {
if (!popover || !overlayRef.current || cssAnchorPositioning) return
try {
if (!overlayRef.current.matches(':popover-open')) {
overlayRef.current.showPopover()
}
} catch {
// Ignore if popover is already showing or not supported
}
}, [popover, cssAnchorPositioning])
- Files reviewed: 6/6 changed files
- Comments generated: 3
| inset: auto; | ||
| margin: 0; | ||
| padding: 0; | ||
| border: 0; | ||
| max-width: none; |
There was a problem hiding this comment.
The [popover]:not([data-anchor-position]) rule sets inset: auto, which overrides the earlier top/left/right/bottom positioning rules for .Overlay. As a result, an Overlay using top/left props may no longer be positioned as intended when popover is present (it also removes the component’s default max-width constraint via max-width: none). Consider overriding the UA popover styles without clobbering the component’s positioning/sizing rules (e.g. explicitly re-apply top/left/right/bottom here, or avoid using the inset shorthand and keep the existing max-width).
| inset: auto; | |
| margin: 0; | |
| padding: 0; | |
| border: 0; | |
| max-width: none; | |
| margin: 0; | |
| padding: 0; | |
| border: 0; |
| children?: React.ReactNode | ||
| className?: string | ||
| responsiveVariant?: 'fullscreen' // we only support fullscreen today but we might add bottomsheet in the future | ||
| popover?: 'auto' | 'manual' |
There was a problem hiding this comment.
Decided not to make it popover by default as I feel like this might consist of more than a minor change.
With it on by default, it seems to work the same, so maybe it's worth enabling it by by default. 🤔 We will need to add popover to instances across Dotcom, which is the con of having it opt-in.
Closes https://github.com/github/primer/issues/6556
Adds popover support to
Overlaycomponent. WithAnchoredOverlaynow supporting popover, theOverlaycomponent should too, to ensure implementations ofAnchoredOverlay+Overlaywork properly.Changelog
New
Overlay(behind theprimer_react_css_anchor_positioningfeature flag)Rollout strategy
Testing & Reviewing
Merge checklist