SearchableChipSelect: Add grouped items support - #80989
Conversation
|
Size Change: 0 B Total Size: 7.77 MB 📦 View Changed
|
| Item.displayName = 'SearchableChipSelect.Item'; | ||
| ChipWithRemove.displayName = 'SearchableChipSelect.ChipWithRemove'; | ||
| Group.displayName = 'SearchableChipSelect.Group'; | ||
| GroupLabel.displayName = 'SearchableChipSelect.GroupLabel'; |
There was a problem hiding this comment.
Why aren't we setting a display name for Collection?
Short answer: I realized we have some systemic problems with the way we assign new display names on re-exports. This is especially noticeable for the Collection subcomponent, but already effects other subcomponents as well.
Longer answer
When higher-level components re-export combobox subcomponents (Item, Group, Collection, etc.) and assign displayName in each layer's index file, two issues stack up:
1. TypeScript vs runtime
forwardRef subcomponents (Item, Group, …) allow displayName assignment. Plain functions like Collection don't — TypeScript errors unless you suppress or cast. forwardRef on Collection doesn't work because base-ui's Collection doesn't take a ref.
2. Shared singleton references
Re-exporting the same imported component and setting displayName in both SearchableChipSelect and SearchableChipSelectControl mutates one function object — only the last assignment wins. That's already true for Item, Group, etc., not just Collection.
Distinct names per layer requires a new component reference each time — a wrapper function or a dedicated file (like SelectControl.Item, which wraps rather than re-exports).
Decision: Skip Collection.displayName for now; treat the singleton / multi-layer naming problem as systemic, not worth solving piecemeal in this PR.
|
Flaky tests detected in cb5302f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/31649300042 Should insert content using the global inserter in |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| * array of groups instead of a flat list of items. | ||
| */ | ||
| items?: Item[]; | ||
| items?: Item[] | ItemGroup[]; |
There was a problem hiding this comment.
According to the new types, items accepts grouped data while children remains optional.
But it also looks like the default renderer skips group objects. But Base UI will still consider the list non-empty, and therefore the empty state will be hidden and the popup appear blank.
Hope it makes sense 😅
There was a problem hiding this comment.
Valid observation, but this is also another conscious decision similar to the point above. We want to avoid conditional types, so the types are kept simple. And the failure mode of forgetting to pass children in the grouped case is non-subtle (i.e. easy to catch). The intent is for Storybook documentation to cover this, rather than contort the internal logic to handle it magically.
There was a problem hiding this comment.
Gotcha. Instead of conditional types, what about a props union like
type FlatItemsProps = {
items?: Item[];
children?: ( item: Item, index: number ) => ReactNode;
};
type GroupedItemsProps = {
items: ItemGroup[];
children: ( group: ItemGroup, index: number ) => ReactNode;
};
type ItemsProps = FlatItemsProps | GroupedItemsProps;
export type SearchableChipSelectProps = Omit<
ComboboxRootProps< Item, true >,
'children' | 'items' | 'multiple'
> &
ItemsProps & {
// Existing common props…
};? Would that prevent the valid-looking blank popup without adding runtime logic without adding too much complexity to the types?
In general, and similarly to what was discussed above, I'd feel better if we tightened this (kind of implicit) contract with redundancy in JSDocs and (dev) runtime errors when the props are not configured in the correct way. These errors may also help agents when building / debugging.
There was a problem hiding this comment.
Conditional shapes in the broad sense like that also complicate type management on the consumer side, often involving type casts once you aren't passing full literal values. With this specifically, it did require extra type casting in the Storybook file, so it's likely that similar inconveniences will be encountered in consumer usage.
I hope the added console warning will suffice for now?
There was a problem hiding this comment.
Right, we can probably keep the logic as-is for now, since the failure will be quite obvious too,.
Maybe we can improve the docs around it?
-items prop: “Grouped items require a custom children renderer.”
-children prop: “Required when items contains groups.”
-Grouped Storybook example: mention that groups have no default renderer
mirka
added
[Type] Breaking Change
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Pre-approving for the sake of speed of iteration.
Good to merge once last round of feedback is addressed 🚀
| export function shouldSkipCollectionEntry( | ||
| entry: Item | ItemGroup, | ||
| creatableItem: CreatableItem | undefined | ||
| ): boolean { | ||
| if ( ! creatableItem ) { | ||
| return false; | ||
| } | ||
|
|
||
| if ( isItem( entry ) ) { | ||
| return isCreatableItem( entry ); | ||
| } | ||
|
|
||
| return ( | ||
| entry.items.length > 0 && | ||
| entry.items.every( ( item ) => isCreatableItem( item ) ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
We currently skip a group only when every item is creatable. I think I identified a small edge case.
If a group contains [ Apple, Create ], children renders Create in the group, and the footer will render it again, meaning there will be two Create options.
Should we remove creatable items from their original groups during normalization? (potentially add a test for this)
| * array of groups instead of a flat list of items. | ||
| */ | ||
| items?: Item[]; | ||
| items?: Item[] | ItemGroup[]; |
There was a problem hiding this comment.
Right, we can probably keep the logic as-is for now, since the failure will be quite obvious too,.
Maybe we can improve the docs around it?
-items prop: “Grouped items require a custom children renderer.”
-children prop: “Required when items contains groups.”
-Grouped Storybook example: mention that groups have no default renderer
| if ( ! hasGroupedItems( items ) ) { | ||
| const flatItems = items as Item[]; | ||
| let lastCreatableIndex = -1; | ||
|
|
||
| for ( let index = flatItems.length - 1; index >= 0; index-- ) { | ||
| if ( isCreatableItem( flatItems[ index ] ) ) { | ||
| lastCreatableIndex = index; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| lastCreatableIndex >= 0 && | ||
| lastCreatableIndex !== flatItems.length - 1 | ||
| ) { | ||
| warning( | ||
| 'SearchableChipSelect: the creatable item should be last in `items` for predictable keyboard navigation.' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we still need this warning branch? normalizeRootItems() should take care of this, there's a chance we don't really need to surface this error to the consumers of the component?
4 participants
What?
Enhances the
SearchableChipSelectprimitive with grouped items support and acreatable: trueitem marker for footer create actions.Why?
Grouped options are a common combobox pattern. Creatable items need to participate in keyboard navigation while staying out of the main list UI.
How?
Group,GroupLabel, andCollectionsubcomponents fromSearchableChipSelect.childrenare passed directly toCombobox.Collection(required for grouped rendering).ItemGrouptyping andisItem/isItemGrouphelpers for groupeditems.creatableItemprop with acreatable: truemarker on an item initems. The component finds the creatable item, excludes it from the main list, moves it to the end of flatitemsfor keyboard order, and renders it in the list footer.itemswithoutchildren, multiple creatable items, creatable not last in flat lists, creatable mixed with regular items in a group).Testing Instructions
npm run test:unit -- packages/ui/src/form/primitives/searchable-chip-select/test/index.test.tsxScreenshots
Grouped items