Add SelectPanel alpha component #1224
Conversation
🦋 Changeset detectedLatest commit: 3d13326 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/EydoRgXfqwLsaA3zRk6MPsGRttac |
|
|
||
| ## Example | ||
|
|
||
| ## Component props |
There was a problem hiding this comment.
- Would you mind opening an issue tracking either a. “Promote
SelectPanelto “beta” status” or b. “Provide examples in theSelectPaneldocs”? - What do you think about incorporating some of the prop documentation from SelectPanel #1071 and DropdownMenu #1070?
src/SelectPanel/SelectPanel.tsx
Outdated
| return | ||
| } | ||
|
|
||
| if (selectionVariant === 'single') { |
There was a problem hiding this comment.
Lines 69–76 could be refactored, depending on https://github.com/primer/components/pull/1224/files#r632789982
src/SelectPanel/SelectPanel.tsx
Outdated
| export interface SelectPanelProps extends Omit<FilteredActionListProps, 'onChange'> { | ||
| open: boolean | ||
| onOpen: (gesture: 'anchor-click' | 'anchor-key-press') => unknown | ||
| onClose: (gesture: 'click-outside' | 'escape') => unknown |
There was a problem hiding this comment.
ActionMenu uses open and setOpen—should we be consistent (either changing ActionMenu to use open/onOpen/onClose, or changing SelectPanel to use open/setOpen)?
There was a problem hiding this comment.
Going with the setOpen approach, passing setOpen(open: boolean, gesture: 'possible gestures here'). We will need to circle back and probably change the DropdownMenu and AnchoredOverlay to have the same API
There was a problem hiding this comment.
Going with the
setOpenapproach, passingsetOpen(open: boolean, gesture: 'possible gestures here').
👍 Sounds good, thanks!
We will need to circle back and probably change the
DropdownMenuandAnchoredOverlayto have the same API.
Great idea! Could you please open an issue to track API consistency updates?
src/SelectPanel/SelectPanel.tsx
Outdated
| onClose: (gesture: 'click-outside' | 'escape') => unknown | ||
| renderAnchor?: <T extends React.HTMLAttributes<HTMLElement>>(props: T) => JSX.Element | ||
| placeholder?: string | ||
| selectedItems: ItemInput[] |
There was a problem hiding this comment.
Non-blocking: I wonder if we should merge items and selectedItems, i.e. add a selected boolean to selected items in items. Since items updates as filters are applied (possible to exclude selected items not matching the current filter), we’d need to refactor some logic/assumptions (e.g. perhaps always retaining the full list of items, and adding an additional matchesFilter boolean).
There was a problem hiding this comment.
This could get really tricky with combined local/remote loaded items. I think the current API provides the most flexibility for now, but I'm willing to reconsider as we work on solidifying the API for all the layered components
src/stories/SelectPanel.stories.tsx
Outdated
| selectedItems={selectedItems} | ||
| onChange={setSelectedItems} | ||
| filterValue={filterValue} | ||
| onFilterChange={onFilterChange} |
There was a problem hiding this comment.
From @dgreif in #1224 (comment):
The current API makes this component ultra controlled, which means flexibility, but also a lot of setup on the consumer side to use it. I think this is evident if you look at the story and tests. There are ~10 props required to use it properly.
It’s interesting to consider how a slotted children API could reduce the props of SelectPanel, e.g. if SelectPanel had a SelectPanel.Filter child, placeholderText, filterValue, and onFilterChange could be moved there. cc @colebemis
There was a problem hiding this comment.
Yeah, I'm interested to see if we could simplify the props by using the content-as-children API.
There was a problem hiding this comment.
This looks great for an alpha component ✨ Thanks for all your hard work and attention to detail on this, @dgreif! ❤️ I'll give this a more thorough review after you've addressed @smockle's thoughtful comments.
Before we promote this to beta, let's explore a content-as-children API to simplify the props.
| loading?: boolean | ||
| placeholderText: string | ||
| onFilterChange: (value: string, e: React.ChangeEvent<HTMLInputElement>) => void | ||
| textInputProps?: Partial<TextInputProps> |
There was a problem hiding this comment.
Should we omit onChange from this type because it could clash with onFilterChange?
This is the initial pass at
SelectPanel(#1071). There are still a number of enhancements that will need to be made, to be implemented in subsequent PRs. Basic scaffolding for docs and tests have been added since this component isAlpha.Also including the
FilteredActionListcomponent. We could incorporate this component directly into theSelectPanel, but I think it's a little easier to separate responsibilities by pulling it out. If we intend to use it elsewhere withoutSelectPanel, we will need to add additional docs and tests.I'm definitely open to discussing the API and implementation. The current API makes this component ultra controlled, which means flexibility, but also a lot of setup on the consumer side to use it. I think this is evident if you look at the story and tests. There are ~10 props required to use it properly.
Screenshots
Merge checklist