-
-
Notifications
You must be signed in to change notification settings - Fork 352
[combobox][select] Add placeholder prop to Value part
#3604
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this even valid? I mean having items array that difference than the Combobox Items rendered?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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