feat: auto-adjust menu width for custom triggers #7710
Conversation
|
@snowystinger This is the fix for #7663. You can merge this into your feature branch for follow up review if you like 🚀 |
| 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.
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.
Thanks, too reliant on eslint already 🤣
Originally, I proposed setting --trigger-width to the width of triggerRef when it's larger than inputRef + buttonRef as a courtesy. This is how people expect it to behave when providing a custom triggerRef.
You are right, maxWidth could be used to clamp smaller values - equivalently we should probably use minWidth here instead of overriding --trigger-width. A bit unfortunate to have 2 resize observers for often times the same ref, but can be improved with a similar concept to idsUpdaterMap at some point 👍
| @@ -43,6 +43,8 @@ const hiddenFragment = typeof DocumentFragment !== 'undefined' ? new DocumentFra | |||
| export function Hidden(props: {children: ReactNode}) { | |||
| let isHidden = useContext(HiddenContext); | |||
| let isSSR = useIsSSR(); | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| let wasSSR = useMemo(() => isSSR, []); | |||
There was a problem hiding this comment.
what was actually happening here? was strictmode running on the server or something?
There was a problem hiding this comment.
See #7663 (comment)
TLDR; During hydration, useIsSSR() remains true, meaning we render into a template element on the first client render. The ref is attached but React can't maintain it as everything is moved to the portal 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 both portal and template the same key, but to no avail also. I suppose this is due to bad support for template elements in general facebook/react#19932
There was a problem hiding this comment.
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
| @@ -43,6 +43,8 @@ const hiddenFragment = typeof DocumentFragment !== 'undefined' ? new DocumentFra | |||
| export function Hidden(props: {children: ReactNode}) { | |||
| let isHidden = useContext(HiddenContext); | |||
| let isSSR = useIsSSR(); | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
| let wasSSR = useMemo(() => isSSR, []); | |||
There was a problem hiding this comment.
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
snowystinger
merged commit 2cb2cf6
into
adobe:fix-ssr-combobox-inner-ref
* fix: SSR Combobox inner ref lost * fix lint * feat: auto-adjust menu width for custom triggers (#7710) * feat: auto-adjust menu width for custom triggers * fix: test assert label compatibility * fix: rules of react * feat: popover minWidth for custom trigger elements * fix: use resize observer * chore: reset combobox story * remove minimum size on popovers * use more stable ref instead of memo * update based on new information * Update to reduce extra renders --------- Co-authored-by: Nikolas Schröter <[email protected]>
Closes #7663 (comment)
Context #7663 (comment)
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: