[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.
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.
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.
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.
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.
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.
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.
It seems that this provider can be rendered by InnerSelectOption
michaldudak
left a comment
There was a problem hiding this comment.
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.Valueplaceholderprop)defaultValue/valuedon't match any of the options)TODO:
useFieldalignMethodprop toalignOptionToTriggerOlivier's edit: the v2 of mui/material-ui#8023 😄