-
-
Notifications
You must be signed in to change notification settings - Fork 46
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
[Select] Create new Select
component
#541
Conversation
Netlify deploy preview |
2384edd
to
c32adfc
Compare
ownerState, | ||
propGetter: (externalProps) => getTriggerProps(getRootTriggerProps(externalProps)), | ||
customStyleHookMapping: commonStyleHooks, | ||
extraProps: otherProps, |
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.
It would be great if the Trigger had a CSS variable with the width of the popup, so it can match it (as in native OS controls)
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.
Do you mean the inverse? --anchor-width
for popup is available. The trigger can't know the width of the popup when it's not mounted
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.
No, I didn't mean the inverse. Native OS selects adapt their width to the longest option. Perhaps this could be an option when keepMounted=true.
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.
That's true, but I'm hesitant to add something that doesn't work by default. Furthermore, it's likely very performance intensive with longer lists to calculate as you'll need to measure every option with getBoundingClientRect
to calculate the largest one. It also doesn't work during SSR, so will cause layout shift. It's likely something that should be hardcoded by the user if necessary.
Could you please review the list of reported Select bugs and set this PR to close the ones that will be fixed by it? |
When you open the select, the page scrollbar disappears causing a layout shift on the right navbar in the doc (https://deploy-preview-541--base-ui.netlify.app/components/react-select/) The |
@flaviendelangle interesting. The |
a0c48cc
to
a4334d8
Compare
I'm wondering if we should prevent scrolling by default at all. I find it annoying when I open a select and something else changes appearance (=the scrollbar disappears). Native implementations differ in this matter - iOS prevents scrolling but doesn't hide the scrollbar, while Windows closes the popup when the page scrolls. I found an issue that's likely related to scroll locking: https://codesandbox.io/p/sandbox/q7qfjg - when you open the select, there's a small layout shift and you're able to scroll the page horizontally until the whole trigger disappears. |
The scroll locking changes we made in #604 prevents layout shift issues from disappearing scrollbars. The scroll lock is necessary with the item align anchoring because you can
This looks like a bug with the .SelectPopup This solves it: min-width: min(var(--available-width), calc(var(--anchor-width) + 20px)); There's a thread discussing default "functional styles" to apply to these elements and an API to override them in a thread in the base-ui channel. This could help prevent certain issues like this, including:
|
The repro was taken directly from the demos, so they'll need to have the styles updated. |
28ffec5
to
a8bed12
Compare
I would assume these two to behave the same way (we have space for the scrolling arrow in both scenarios).
|
If the Fair point! Can change this, not sure if we should provide an option if so. This seems to be a new pattern or suggestion that I haven't seen before, and the native select doesn't appear to do this. It doesn't seem critical to implement for now. Yes, Home/End are handled |
|
||
const listItem = useCompositeListItem({ label }); | ||
const { activeIndex, selectedIndex, setActiveIndex } = useSelectIndexContext(); | ||
const { getItemProps, setOpen, setValue, open, selectionRef, typingRef, valuesRef, popupRef } = |
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.
Can this be moved to InnerSelectOption? It seems that these values don't change on each option highlight (except maybe getItemProps
?)
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.
Consuming the root context inside the memoized component is risky as any time the root context changes it will cause every option to re-render. Even if they don't change on each option highlight, they can change on other unrelated state changes. Minor refactorings could introduce issues unknowingly as well.
); | ||
|
||
return ( | ||
<SelectOptionContext.Provider value={contextValue}> |
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.
It seems that this provider can be rendered by InnerSelectOption
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 performance of large lists is still not quite good. I played a bit with the profiler and identified that quite a lot of time is spent in FloatingFocusManager's handleMutation (https://github.com/floating-ui/floating-ui/blob/master/packages/react/src/components/FloatingFocusManager.tsx#L651). I initially thought that the slowdown may be caused by focusing each item on hover, but Radix does is as well and yet their Select feels snappier. IMO we can merge this PR so that this branch doesn't live for too long, but we should then investigate what can be done to make it even faster.
Preview: https://deploy-preview-541--base-ui.netlify.app/components/react-select/
Select.Value
placeholder
prop)defaultValue
/value
don't match any of the options)TODO:
useField
alignMethod
prop toalignOptionToTrigger
Olivier's edit: the v2 of mui/material-ui#8023 😄