-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: auto-adjust menu width for custom triggers #7710
Changes from 2 commits
e821fa8
14ff35f
4399c38
fd67389
0e8bbec
bf9c710
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 |
---|---|---|
|
@@ -17,7 +17,7 @@ import {forwardRefType, RefObject} from '@react-types/shared'; | |
import {OverlayArrowContext} from './OverlayArrow'; | ||
import {OverlayTriggerProps, OverlayTriggerState, useOverlayTriggerState} from 'react-stately'; | ||
import {OverlayTriggerStateContext} from './Dialog'; | ||
import React, {createContext, ForwardedRef, forwardRef, useContext, useRef, useState} from 'react'; | ||
import React, {createContext, ForwardedRef, forwardRef, useContext, useMemo, useRef, useState} from 'react'; | ||
import {useIsHidden} from '@react-aria/collections'; | ||
|
||
export interface PopoverProps extends Omit<PositionProps, 'isOpen'>, Omit<AriaPopoverProps, 'popoverRef' | 'triggerRef' | 'offset' | 'arrowSize'>, OverlayTriggerProps, RenderProps<PopoverRenderProps>, SlotProps { | ||
|
@@ -91,6 +91,22 @@ export const Popover = /*#__PURE__*/ (forwardRef as forwardRefType)(function Pop | |
let isExiting = useExitAnimation(ref, state.isOpen) || props.isExiting || false; | ||
let isHidden = useIsHidden(); | ||
|
||
// To force a menu width lower than the triggerWidth, use CSS variables. | ||
let style = useMemo(() => { | ||
let triggerWidth = props.style?.['--trigger-width']?.replace('px', ''); | ||
|
||
if (props.triggerRef?.current && !isNaN(Number(triggerWidth))) { | ||
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. I think this breaks the rules for reading from a ref during render, would need to read the ref during an effect, then store the width in state. I'm not entirely sure I follow what's going on here. To clamp to whatever value is passed in that is smaller than --trigger-width, couldn't we use maxWidth? 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. Thanks, too reliant on eslint already 🤣 Originally, I proposed setting You are right, |
||
let triggerRect = props.triggerRef.current.getBoundingClientRect(); | ||
let menuWidth = triggerRect.right - triggerRect.left; | ||
|
||
if (menuWidth > Number.parseFloat(triggerWidth)) { | ||
return {...props.style, '--trigger-width': menuWidth + 'px'}; | ||
} | ||
} | ||
|
||
return props.style; | ||
}, [props.triggerRef, props.style]); | ||
|
||
// If we are in a hidden tree, we still need to preserve our children. | ||
if (isHidden) { | ||
let children = props.children; | ||
|
@@ -114,6 +130,7 @@ export const Popover = /*#__PURE__*/ (forwardRef as forwardRefType)(function Pop | |
return ( | ||
<PopoverInner | ||
{...props} | ||
style={style} | ||
triggerRef={props.triggerRef!} | ||
state={state} | ||
popoverRef={ref} | ||
|
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.
what was actually happening here? was strictmode running on the server or something?
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.
See #7663 (comment)
TLDR; During hydration,
useIsSSR()
remainstrue
, meaning we render into atemplate
element on the first client render. The ref is attached but React can't maintain it as everything is moved to theportal
in subsequent renders, in this case caused by the state update of the parent. This change forces a consistent strategy.I tried using
portal
for the initial render as well as assigning bothportal
andtemplate
the same key, but to no avail also. I suppose this is due to bad support for template elements in general facebook/react#19932There 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 see, so now it will always render into template if it's an SSR app? we'll have to see if there's a performance hit for that
the reason for doing it in the portal was so we could use a super slimmed down document