-
-
Notifications
You must be signed in to change notification settings - Fork 266
[select] Add open state for Select.Icon and fix ref type
#2714
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: |
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Bundle size report
|
| value, | ||
| placeholder: value === null, | ||
| }), |
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.
One potential issue with this is with item objects as values (#2704), where the null item is an object.
Two use cases:
- The
nullitem object isn't in the popup list, so this isn't a concern (nullis the default and will match like a real placeholder; you can't "deselect" to go back to the placeholder via the popup items list) - The
nullitem object is in the popup list, and you need to usedefaultValue={items[0]}to ensure it's initially selected, since it appears in the list. Thendata-placeholderwon't be added.
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.
Thank you for the detailed feedback!
To make sure I understand correctly, are you referring to a use case like the one below, where the placeholder is an item object with an empty string as its value?
const items = [
{ country: 'Select Country', code: '' },
{ country: 'United States', code: 'US' },
{ country: 'Canada', code: 'CA' },
{ country: 'Australia', code: 'AU' },
];
<Select.Root
name="country"
defaultValue={items[0]}
itemToStringLabel={(item) => item.country}
itemToStringValue={(item) => item.code}
>
<Select.Trigger>
<Select.Value />
</Select.Trigger>
<Select.Portal>
<Select.Positioner>
<Select.Popup>
{items.map((it) => (
<Select.Item key={it.code} value={it}>
{it.country}
</Select.Item>
))}
</Select.Popup>
</Select.Positioner>
</Select.Portal>
</Select.Root>If so, what are your thoughts on using the existing serializedValue from SelectRoot to handle this?
const serializedValue = React.useMemo(() => {
if (isMultiple && Array.isArray(value) && value.length === 0) {
return '';
}
return stringifyAsValue(value, itemToStringValue);
}, [isMultiple, value, itemToStringValue]);
const state: SelectValue.State = React.useMemo(
() => ({
value,
placeholder: !serializedValue, // An empty string will correctly evaluate to `true`
}),
[value, serializedValue],
);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.
Yeah, effectively - an empty string could be considered a placeholder as well:
const items = [
{ value: null, label: 'select' },
{ value: 'apple', label: 'Apple' },
]
<Select.Root items={items} defaultValue={items[0]}>
<Select.Item value={item}> {/* full object when mapping */}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.
open state for Select.Icon and fix ref type
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.
Looks good, thanks for the contribution
pnpm docs:api needs to be run though. SelectIconDataAttributes file also needs to be created (with that command run) so it displays the data-popup-open attr in a table in the docs
|
@atomiks I added |
Add the data-placeholder attribute to Select.Value when a placeholder item with no value is selected. This allows for applying custom styles. (d3eb638)split this out into a [select] add data-placeholder #2737