[combobox][select] Add placeholder prop to Value part#3604
Conversation
commit: |
Bundle size report
Check out the code infra dashboard for more information about this PR. |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
b95e935 to
ee223c0
Compare
c8029dd to
5e4fa02
Compare
mnajdova
left a comment
There was a problem hiding this comment.
Logic looks good, left few comments/questions to be resolved before merging.
| <Combobox.Positioner> | ||
| <Combobox.Popup> | ||
| <Combobox.List> | ||
| <Combobox.Item value={null}>None</Combobox.Item> |
There was a problem hiding this comment.
Is this even valid? I mean having items array that difference than the Combobox Items rendered?
There was a problem hiding this comment.
Technically yes, this pattern is currently half-broken but is specified throughout the tests. This PR gives it full support: #3594
| const items = useStore(store, selectors.items); | ||
| const itemToStringLabel = useStore(store, selectors.itemToStringLabel); | ||
| const serializedValue = useStore(store, selectors.serializedValue); | ||
| const hasSelectedValue = useStore(store, selectors.hasSelectedValue); |
There was a problem hiding this comment.
There is a lot of repetition here and in the combobox, does it makes sense to extract this in a usePlaceholder hook, or is it too much of an abstraction considering we use different stores? 😕
There was a problem hiding this comment.
I think "rule of three" for DRY works here and this is only 2 separate places
c62352e to
b0baa48
Compare
d017b70 to
951c7ab
Compare
951c7ab to
ebf30a7
Compare
6b426fd to
0c14c90
Compare
0c14c90 to
b0470ed
Compare
When a
nullitem isn't rendered in the list,placeholderacts as a typical placeholder, though it cannot be cleared.Issues when trying to use a native
<label>, which prompted the demos to useField:<label>opens the popup, but this doesn't match native<select>behavior. Trying to workaround this is quite expensive and heavy-handed. On the other hand,<Field.Label>links to the hidden input and only gives focus to the trigger and matches native behavior..Trigger:hover, which unlike Checkbox/Radio, doesn't really look or feel right.<Field.Label>also avoids this.Select: https://deploy-preview-3604--base-ui.netlify.app/react/components/select
Combobox: https://deploy-preview-3604--base-ui.netlify.app/react/components/combobox#input-inside-popup
Closes #3720