Select primitive #38
Select primitive #38
Conversation
…ibility, and examples
…erns, and form integration
…lity and SSR support
… props, and accessibility
…ate + accessibility handling
🦋 Changeset detectedLatest commit: 62d6ff2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 27 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…to ListBoxItem for consistency
…es, and enhance examples
…ed docs, and empty state
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…nditional rendering
kawikabader
force-pushed
the
select-primitive
branch
from
a2f405b to
08a0427
Compare
packages/pressable/src/index.tsx
Outdated
| const { pressProps, isPressed } = usePress({ ...mergedProps, ref }); | ||
|
|
||
| // Second pass: get apply function with interaction state for render props | ||
| const { props, apply } = useProps(args, { isHovered, isFocused, isFocusVisible, isPressed }); |
There was a problem hiding this comment.
This seems like we need to re-think useProp as it pertains to exposing state to the prop functions.
There was a problem hiding this comment.
useProp is defined earlier in the function, and when the state has finally been determined....it has to be re-called. Just doesn't seem like an ideal flow.
There was a problem hiding this comment.
The two-pass pattern exists because of ordering requirements:
Pass 1 merges slot props so React Aria hooks see overrides like isDisabled.
Pass 2 provides the apply() function with interaction state for render props.
We can't do both in one pass because we need mergedProps to GET the state, but we need the state to execute render props.
I'm open to other ideas if you have them though.
packages/button/src/index.tsx
Outdated
| * Resolves button children - either renders the function with state or returns static content. | ||
| * @internal | ||
| */ | ||
| export function resolveChildren( |
There was a problem hiding this comment.
Doesn't useProp for the children prop already do this?
There was a problem hiding this comment.
useProps render‑prop contract is:
if (isRenderProp(name, value)) return value({
props, slots, state, original
});In Button we call useProps(args, {}), so props.children (via the proxy) would get { props, slots, state: {}, original }, not { isPressed, isHovered, isFocused, isFocusVisible } from React Aria. That’s why we intentionally bypass props.children and call resolveChildren(args.children, { isPressed, ... }) instead.
There was a problem hiding this comment.
If this is a problem with the useProps API, then that API should be adjusted. We should not be making this component behave differently.
I'm missing the part why this change is needed?
There was a problem hiding this comment.
Alrighty, I've removed the resolveChildren helper function.
The current implementation handles children inline without introducing a new abstraction:
// Execute children render prop explicitly (before useProps to prevent incorrect execution)
const content = typeof args.children === 'function' ? args.children(renderState) : args.children;
// Second pass: apply user props with render state via apply()
const { apply } = useProps(args, renderState);Does this feel more appropriate here?
packages/select/src/select.tsx
Outdated
| * @returns The render function from the ListBox, or undefined if not found | ||
| * @internal | ||
| */ | ||
| function extractListBoxRenderFunction(children: ReactNode): ((item: unknown) => ReactNode) | undefined { |
There was a problem hiding this comment.
This function seems very fragile. It expects a specific composition pattern, and extracting the render from children, is this necessary?
There was a problem hiding this comment.
So useSelectState needs the children(item) => ReactNode render fn at the Select level for items-based (dynamic) collections, but our slot API hides it inside <ListBox slot="listbox">. extractListBoxRenderFunction walks the known slot structure, finds the listbox slot, and lifts its render fn so we can keep the nice slot-based API without inventing a parallel renderItem prop on Select. It’s fragile only to the extent that our public slot contract is: Select requires exactly one ListBox with slot="listbox".
There was a problem hiding this comment.
I've removed extractListBoxRenderFunction entirely and implemented an explicit renderItem prop on Select instead.
New pattern:
<Select
items={fruits}
renderItem={(fruit) => <ListBoxItem id={fruit.id}>{fruit.name}</ListBoxItem>}
>
<Button slot="trigger">...</Button>
<Popover slot="popover">
<ListBox slot="listbox" /> {/* presentational only */}
</Popover>
</Select>The renderItem prop is now required (type-enforced) when items is provided, making the contract explicit and removing all slot traversal logic. The ListBox slot becomes purely presentational for dynamic collections.
Does this feel like a better solution?
There was a problem hiding this comment.
Purely based on the example you have above. I'd be confused about the purpose of <Select renderItem/> in relation to the items I'd expect in the <Listbox/>. What's keeping us from using the same construction that Adobe has?
<Select>
<Label />
<Button>
<SelectValue />
</Button>
<Text slot="description" />
<FieldError />
<Popover>
<ListBox />
</Popover>
</Select>There was a problem hiding this comment.
Yeah that's a solid point. I've gone ahead and removed items and renderItem from Select and moved them to ListBox.
Now matches Adobe's pattern:
<Select>
<Button slot="trigger">...</Button>
<Popover slot="popover">
<ListBox slot="listbox" items={fruits}>
{(fruit) => <ListBoxItem id={fruit.id}>{fruit.name}</ListBoxItem>}
</ListBox>
</Popover>
</Select>
Select is now purely a coordinator (overlay, state, forms). ListBox owns its collection.
- Resolved conflicts in package.json files - Updated select package dependencies to match latest versions - Removed unused @bento/pressable dependency from button - Regenerated package-lock.json
- Simplified ButtonWithRenderPropExample to remove untestable isPressed branch - Added test for hover state in render prop example - All tests now pass with 100% coverage
akazemier-godaddy
left a comment
akazemier-godaddy
left a comment
There was a problem hiding this comment.
Just going to hard block on this. Seems like we're going in a completely wrong direction here by suddenly breaking our interface contracts for no reason.
packages/button/src/index.tsx
Outdated
| * Resolves button children - either renders the function with state or returns static content. | ||
| * @internal | ||
| */ | ||
| export function resolveChildren( |
There was a problem hiding this comment.
If this is a problem with the useProps API, then that API should be adjusted. We should not be making this component behave differently.
I'm missing the part why this change is needed?
…nd improved prop handling
| state?: S | ||
| ): Returns; | ||
| export function useProps(...rest: unknown[]): Returns { | ||
| export function useProps(...rest: any[]): Returns { |
There was a problem hiding this comment.
There is a lot changing here, types being removed, and no tests.
Can you describe what specifically is changing and why?
| <Select items={[]} renderEmptyState={() => <div data-testid="empty">No items</div>}> | ||
| <Select | ||
| items={[]} | ||
| renderItem={renderFruitItem} |
There was a problem hiding this comment.
Why do we have a renderItem prop? Wouldn't this be handled by the Listbox render function?
| const [open, setOpen] = useState(false); | ||
| const [mounted, setMounted] = useState(false); | ||
| const triggerRef = useRef<HTMLButtonElement>(null); | ||
| const [trigger, setTrigger] = useState<HTMLButtonElement | null>(null); |
There was a problem hiding this comment.
Why the use of useState to set the button ref?
… and mergeRefs utility
| }; | ||
|
|
||
| // Execute children render prop explicitly (before useProps to prevent incorrect execution) | ||
| const content = typeof args.children === 'function' ? args.children(renderState) : args.children; |
There was a problem hiding this comment.
So here we are deviating from how Bento handles the "function" props, no? https://github.com/godaddy/bento/blob/main/packages/use-props/src/index.ts#L221-L226
// from this
<button>(({ props, state }) => ...)</button>
// to this
<button>(({ isHovered, isPressed, ... }) => ...)</button>Not sure if exposing those states in the second argument of useProps is the right call but I'd look into it:
const { props: mergedProps, ref: mergedRef } = useProps(args, renderState, forwardedRef);
Summary
Introduces the
@bento/selectcomponent.This PR also includes necessary updates to
@bento/button,@bento/pressable,@bento/listbox,@bento/use-props,@bento/text, and@bento/overlaythat were discovered during Select development.Changes Made
New:
@bento/selecttrigger,popover,listbox,value,label,description,errorslotsitemsprop withNode<T>.valuepattern for rich data accessListBoxSectiongrouping supportrenderEmptyStatepropisOpen/defaultOpen)<select>for native submissionusePopoverwith flip, offset, and container padding optionsRefactored:
@bento/buttonuseButton,useFocusRing,useHover) instead of wrappingPressableusePropscall with explicit slot prop passthrough for ARIA attributesButtonRenderPropsand render function children supportforwardRefsupportRefactored:
@bento/use-propsrenderPropnow correctly handles falsy values (false,0,'') usinginoperatorRefactored:
@bento/pressableusePropsto enable render propsFixed:
@bento/textasprop type fromstringtoReact.ElementTypeFixed:
@bento/overlayrefpattern instead ofchildRefCleaned up:
@bento/listboxvalueprop fromListBoxItemProps(React Aria handles this viaNode<T>.value)Test Plan
renderPropfalsy value handlingnpm test)Checklist
npm run changesetif this affects packages)