-
Notifications
You must be signed in to change notification settings - Fork 534
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Would you mind opening an issue tracking either a. “Promote
SelectPanel
to “beta” status” or b. “Provide examples in theSelectPanel
docs”? - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going with the
setOpen
approach, passingsetOpen(open: boolean, gesture: 'possible gestures here')
.
👍 Sounds good, thanks!
We will need to circle back and probably change the
DropdownMenu
andAnchoredOverlay
to 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we omit onChange
from this type because it could clash with onFilterChange
?
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 for alpha 👍 Let's experiment with the API a bit more before promoting to beta.
Excellent work, @dgreif ✨
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
FilteredActionList
component. 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