-
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
e949fce
b3e657c
f455610
bdc06db
09acc89
ea2e639
414cc91
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' | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
|
||
| 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() | ||
|
TylerJDev marked this conversation as resolved.
|
||
| } | ||
| } catch { | ||
| // Ignore if popover is already showing or not supported | ||
| } | ||
| }, [popover, cssAnchorPositioning]) | ||
|
TylerJDev marked this conversation as resolved.
|
||
|
|
||
| // 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={cssAnchorPositioning ? popover : undefined} | ||
| {...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).