-
Notifications
You must be signed in to change notification settings - Fork 658
Overlay: Add popover support #7751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| Overlay: Adds popover API support |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,7 @@ type BaseOverlayProps = { | |
| children?: React.ReactNode | ||
| className?: string | ||
| responsiveVariant?: 'fullscreen' // we only support fullscreen today but we might add bottomsheet in the future | ||
| popover?: 'auto' | 'manual' | ||
| } | ||
|
|
||
| type OwnOverlayProps = Merge<StyledOverlayProps, BaseOverlayProps> | ||
|
|
@@ -186,6 +187,7 @@ const Overlay = React.forwardRef<HTMLDivElement, internalOverlayProps>( | |
| visibility = 'visible', | ||
| width = 'auto', | ||
| responsiveVariant, | ||
| popover, | ||
| ...props | ||
| }, | ||
| forwardedRef, | ||
|
|
@@ -229,6 +231,20 @@ const Overlay = React.forwardRef<HTMLDivElement, internalOverlayProps>( | |
| ) | ||
| }, [anchorSide, slideAnimationDistance, slideAnimationEasing, visibility]) | ||
|
|
||
| // Show popover when using the Popover API | ||
| // Skip if CSS anchor positioning is enabled (handled by AnchoredOverlay) | ||
| useLayoutEffect(() => { | ||
| if (!popover || !overlayRef.current || cssAnchorPositioning) return | ||
|
|
||
| try { | ||
| if (!overlayRef.current.matches(':popover-open')) { | ||
| overlayRef.current.showPopover() | ||
|
Comment on lines
+236
to
+241
|
||
| } | ||
| } catch { | ||
| // Ignore if popover is already showing or not supported | ||
| } | ||
| }, [popover, cssAnchorPositioning]) | ||
|
Comment on lines
+234
to
+246
|
||
|
|
||
| // To be backwards compatible with the old Overlay, we need to set the left prop if x-position is not specified | ||
| const leftPosition = left === undefined && right === undefined ? 0 : left | ||
|
|
||
|
|
@@ -243,6 +259,7 @@ const Overlay = React.forwardRef<HTMLDivElement, internalOverlayProps>( | |
| height={height} | ||
| visibility={visibility} | ||
| data-responsive={responsiveVariant} | ||
| popover={popover} | ||
| {...props} | ||
| /> | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
[popover]:not([data-anchor-position])rule setsinset: auto, which overrides the earliertop/left/right/bottompositioning rules for.Overlay. As a result, anOverlayusingtop/leftprops may no longer be positioned as intended whenpopoveris present (it also removes the component’s defaultmax-widthconstraint viamax-width: none). Consider overriding the UA popover styles without clobbering the component’s positioning/sizing rules (e.g. explicitly re-applytop/left/right/bottomhere, or avoid using theinsetshorthand and keep the existingmax-width).