[combobox][select] Add placeholder prop to Value part
#3604
Conversation
atomiks
added
component: select
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. |
atomiks
force-pushed
the
feat/combobox-select-placeholder
branch
from
b95e935 to
ee223c0
Compare
atomiks
force-pushed
the
feat/combobox-select-placeholder
branch
2 times, most recently
from
c8029dd to
5e4fa02
Compare
mnajdova
left a comment
mnajdova
left a comment
There was a problem hiding this comment.
Logic looks good, left few comments/questions to be resolved before merging.
| */ | ||
| listEmpty: boolean; | ||
| /** | ||
| * Whether the combobox doesn't have a value. |
There was a problem hiding this comment.
| * Whether the combobox doesn't have a value. | |
| * Indicates that combobox doesn't have a value. |
To read better.
There was a problem hiding this comment.
I would rename all state booleans to use this format :D
There was a problem hiding this comment.
Other boolean state attrs use Whether so I think that's correct for consistency. Indicates on the other hand is used when there is a value alongside the attribute. The CSS vars use "Present when..." for boolean attributes, but that's only because the attribute doesn't exist when false
| <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
atomiks
force-pushed
the
feat/combobox-select-placeholder
branch
3 times, most recently
from
c62352e to
b0baa48
Compare
atomiks
force-pushed
the
feat/combobox-select-placeholder
branch
3 times, most recently
from
39c2f72 to
d017b70
Compare
atomiks
force-pushed
the
feat/combobox-select-placeholder
branch
from
d017b70 to
951c7ab
Compare
3 participants
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