-
-
Notifications
You must be signed in to change notification settings - Fork 321
[select] Add multiple prop
#2173
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
Conversation
commit: |
Bundle size report
|
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
93690f6 to
997ff3f
Compare
8d5e9ba to
901ea82
Compare
e342325 to
c7ffa35
Compare
81dfec0 to
a814f66
Compare
a814f66 to
452ccfa
Compare
Signed-off-by: atomiks <cc.glows@gmail.com>
| // Pre-compute a value → label map to enable O(1) look-ups when deriving the | ||
| // display label(s). | ||
| const valueLabelMap = React.useMemo(() => { | ||
| if (!items) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (Array.isArray(items)) { | ||
| const map = new Map<any, React.ReactNode>(); | ||
| for (const { value: itemValue, label } of items) { | ||
| map.set(itemValue, label); | ||
| } | ||
| return items[value]; | ||
| return map; | ||
| } | ||
|
|
||
| return new Map<any, React.ReactNode>(Object.entries(items)); | ||
| }, [items]); |
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 don't insist on it, but this is a case where the store and reselect can help. You can put derived values in memoized selectors. Those are more efficient than React's useMemo.
const itemsSelector = state => state.items
const valueSelector = state => state.value
const multipleSelector = state => state.multiple
const valueLabelMapSelector = createSelectorMemoized(
itemsSelector,
items => { /* ... */ }
)
const labelFromItems = createSelectorMemoized(
valueSelector,
multipleSelector,
valueLabelMapSelector,
(value, multiple, valueLabelMap) => { /* ... */ }
)That being said, also note that I'm not sure this code is actually beneficial as a performance optimization. The main issue is that we're adding the cost of building a Map (an O(n) cost) for each Select on mount. If you have a page with a large number of those components, you're actually increasing the page-load time.
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.
Building a map could potentially be beneficial (or at least not harmful) when items is an array. Without the map, we'd need to find the label to display in the array, what also is O(n). However, when items is an object, it seems pointless.
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 changed it so it only creates a map under this condition:
Array.isArray(items) && Array.isArray(value)
As this is the one that creates a loop within a loop to find the label for each value. It should also not be so common to have many of these on one page. I also think most will be using a custom renderer (children function) instead of using the default comma-separated behavior, which sidesteps this
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.
Building a map could potentially be beneficial (or at least not harmful)
That's false. Building a map upfront means you're forcing an O(n) cost to happen at mount time. This only saves time assuming that:
- The user changes the value of the select multiple times (which I could argue is a rarer use-case than only setting its value once)
- Every select on a page gets its value changed (otherwise, you're paying an
O(n)cost for no reason), which is not necessarily true
It should also not be so common to have many of these on one page.
I feel like you're consistently thinking only of simple cases. If you want to build a general-purpose framework that can be used for a wide variety of cases, you need to consider complex/edge cases as well. For example, if a user wants Select in datagrid cells, I can guarantee you I'm going to be unhappy about the mount-time performance cost of creating those Map:s while the user is scrolling the grid - it directly competes with the virtualization logic for the available CPU time.
An unopened Select is basically a button component. It should not have an O(n) cost associated with it at mount-time.
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.
And the reason why I said store selectors can be more efficient is that they would allow you to make that O(n) computation lazy, delaying it to until the map is needed for the first time. React.useMemo() forces the O(n) immediately. You would get all the benefits and no downsides if you moved that computation to a store selector (provided you don't useSelector it but call the selector directly when you need it).
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 latest commit removes the map entirely as they always need to use a custom renderer for multiple selection. They'll use their own lookup map to retrieve each label for the values
| "description": "Determines if the select enters a modal state when open.\n- `true`: user interaction is limited to the select: document page scroll is locked and and pointer interactions on outside elements are disabled.\n- `false`: user interaction with the rest of the document is allowed." | ||
| }, | ||
| "multiple": { | ||
| "type": "false", |
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 not quite right. I'll see what's going on in the API extractor
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.
#2104 should fix it.
| // Pre-compute a value → label map to enable O(1) look-ups when deriving the | ||
| // display label(s). | ||
| const valueLabelMap = React.useMemo(() => { | ||
| if (!items) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (Array.isArray(items)) { | ||
| const map = new Map<any, React.ReactNode>(); | ||
| for (const { value: itemValue, label } of items) { | ||
| map.set(itemValue, label); | ||
| } | ||
| return items[value]; | ||
| return map; | ||
| } | ||
|
|
||
| return new Map<any, React.ReactNode>(Object.entries(items)); | ||
| }, [items]); |
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.
Building a map could potentially be beneficial (or at least not harmful) when items is an array. Without the map, we'd need to find the label to display in the array, what also is O(n). However, when items is an object, it seems pointless.
cc21462 to
cd589f6
Compare
cd589f6 to
d892008
Compare
|
Hey y'all - is there any update on this? What needs to be done to get this merged? If needed, I can pick this up and help fix the relevant issues... Would really love to have this feature as it's very relevant for basically every project where I use a |
michaldudak
left a comment
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.
Aside from #2173 (comment), this looks OK. If you merge this before #2104, I'll fix the docs in that PR.
afad197 to
96f2388
Compare
|
Note on hidden From what I can tell the hidden |
eaee0c8 to
dc9da7e
Compare
dc9da7e to
136877c
Compare
Closes #1956
Also solve radix-ui/primitives#1270
https://deploy-preview-2173--base-ui.netlify.app/react/components/select#multiple-selection