-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(Popover): use Bootstrap default offsets #5663
Conversation
src/useOverlayOffset.tsx
Outdated
) { | ||
// Default popover offset. | ||
// https://github.com/twbs/bootstrap/blob/5c32767e0e0dbac2d934bcdee03719a65d3f1187/js/src/popover.js#L28 | ||
return [0, 8]; |
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.
I think we should expose this via a module constant, maybe in Popover, just to ensure users have access to these hard coded values in case they change
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.
Done. Had to move ArrowProps
and Placement
types from Overlay
over to types.tsx
to fix a circular dependency warning.
@@ -166,7 +166,7 @@ function Overlay({ | |||
...outerProps | |||
}: OverlayProps) { | |||
const popperRef = useRef({}); | |||
const [ref, marginModifiers] = usePopperMarginModifiers(); | |||
const [ref, modifiers] = useOverlayOffset(); |
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.
hmm this API is unfortunate...ideally you'd want the the Popover component itself to specify this, might be feasible with context, but i'd be afraid of timing issues. Lets make sure we aren't calling this hook "public" in case we want to remove it later
Add offset constant into popover Move overlay types into another types.tsx to prevent circular dependency warning
For bootstrap 5
Popovers are not aligned due to a missing default offset found in upstream bootstrap:
https://github.com/twbs/bootstrap/blob/5c32767e0e0dbac2d934bcdee03719a65d3f1187/js/src/popover.js#L28
The approach here uses a
useOverlayOffset
hook to add the proper offset if it's a popover, otherwise default [0, 0].This is the last usage of
usePopperMarginModifiers
, so we can remove that in this PR after #5662 is merged.