-
Notifications
You must be signed in to change notification settings - Fork 0
Select Primitive PDR #10
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
base: main
Are you sure you want to change the base?
Conversation
…ibility, and examples
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.
REMOVED
|
| SelectLabel | ||
| } from '@bento/select'; | ||
|
|
||
| <Select> |
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 was thinking this would take a more re-usable component direction. With the slots, we should be able to pass down any props needed to properly align the components.
<Select>
<Button slot="trigger"><Text slot="value" /></Button>
<Popover slot="popover">
<Listbox slot="list">
<ListboxItem>My First Value</ListboxItem>
</ListBox>
</Popover>
<Text slot="description" />
<Text slot="errorMessage" />
</Select>I believe this approach simplifies the composition, and more freely allows custom components to be used ie <MyCustomContainer slot="description" />.
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.
A few design questions @jsmith9-godaddy :
-
For the
popoverslot, should Select calluseOverlay/useOverlayPositioninternally and merge those props onto the slotted component? Or should users provide a Popover that handles overlay/positioning? -
React Aria's
CollectionBuilderusescreateLeafComponent(ItemNode, ...)to connect items. BothListBoxItemandSelectOptionuse this pattern. If we useListBoxItemdirectly withslot="list", SelectOption'svalueprop convenience (mapping toid) would be lost. Should we:- Use
ListBoxItemdirectly and require users to provideid? - Or keep SelectOption for the
value→idmapping?
- Use
-
With slot-based composition, compile-time checks are harder (ensuring
triggerPropsgo to a button-like component,menuPropsto a listbox-like component). Document expected prop types per slot, or add typed helpers/wrappers? -
Bento's slot system uses the
slotsprop for customization (e.g.,slots={{ 'trigger.value': {...} }}), while this finds children by theirslotprop. These are different patterns. Should this be:- A separate pattern specific to Select?
- Or extend the slot system to support both patterns?
-
React Aria provides
mergeRefsfrom@react-aria/utils(used in checkbox, radio, pressable). Should we use that instead of a custom implementation for consistency?
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.
- Yes, the idea is the root component
Selectwould be the coordinator that applies all necessary props to the appropriate slot - React Aria follows a similar pattern to what I described by using Listbox: https://react-spectrum.adobe.com/react-aria/Select.html#example
- Could we not type the slots with interfaces that a consumer could use if they wanted to create a custom component for that slot?
- Extend the slot system to fit the composition strategy we want to implement
- Yes, utilize any utils from
@react-aria/utilsthat we need
rendered