Uh oh!
There was an error while loading. Please reload this page.
SelectPanel: Add variant=modal - #5198
Conversation
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
|
| onCancel={() => setSelected(initialItems)} | ||
| // backward compatible API choice | ||
| // TODO: improve this API, rename it to a single secondaryAction or secondaryFooterSlotSomething | ||
| footer={<Button size="small">Edit labels</Button>} |
There was a problem hiding this comment.
non blocking improvement 1/2: While footer is backward compatible, it's not obvious what it means. What we want here is a "secondary action".
Looking at usage (3 instances), we have
- Button
- LinkButton
- custom FooterActions component that has a spinner or an error message! (these cases are filling in for missing features and we should be able to simplify them with error messages and loading states)
| > | ||
| Cancel | ||
| </Button> | ||
| {/* TODO: loading state for save? */} |
There was a problem hiding this comment.
non blocking improvement 2/2: Once we have loading states, add one for "saving"
broccolinisoup
left a comment
There was a problem hiding this comment.
Thanks for working on this super quickly 🔥 Let a few questions. curious to hear your thoughts
| onSelectedChange: (selected: ItemInput[]) => void | ||
| } | ||
| type OpenGestures = 'anchor-click' | 'anchor-key-press' |
| <h1>Multi Select Panel as Modal</h1> | ||
| <SelectPanel | ||
| variant="modal" | ||
| onCancel={() => setSelected(initialItems)} |
There was a problem hiding this comment.
I wonder if it is doable/makes sense to handle the cancel in the component? 🤔 We can tae a call back in case there is anything else needs to be done after the cancel. Curious to hear what you think
There was a problem hiding this comment.
I would like that a lot. A combination of onSubmit and onCancel so that developers don't have to take care of transient state
buuuuut that doesn't match what we have right now. onSelectedChange is fired immediately when an item is selected and the developer are expected to keep track of selected items and pass them back to SelectPanel 🤷
I'd like to explore an API that abstracts the transient state but that is likely going to be a breaking change, so probably as a bonus task right at the end of the project
There was a problem hiding this comment.
buuuuut that doesn't match what we have right now. onSelectedChange is fired immediately when an item is selected and the developer are expected to keep track of selected items and pass them back to SelectPanel 🤷
Yeah, I see! We don't have much control over the selected items, and it would probably violate the pattern if we try managing the state of the "selected" just for cancel
I'd like to explore an API that abstracts the transient state but that is likely going to be a breaking change, so probably as a bonus task right at the end of the project
That would be interesting to explore! Yeah, no worries at all.
| | { | ||
| // do we need keep the old variants for backward-compat? | ||
| // there isn't any usage in dotcom | ||
| variant: 'anchored' | FilteredActionListProps['variant'] |
There was a problem hiding this comment.
do we need keep the old variants for backward-compat? there isn't any usage in dotcom
I think we should. Although I am not sure if should merge the old variants and the new ones since they seem different variants to me. The new one is more of a type (modal, anchored) and the other (full and inset ) feels related to the stylings of the panel.
One idea might be (For the sake of keeping things uncomplicated) we can merge them and immediately deprecated the old variants?
Curious to hear your thoughts
There was a problem hiding this comment.
Although I am not sure if should merge the old variants and the new ones since they seem different variants to me
That's very true.
I personally would like to get rid of the old ones because it's a design changing option that has leaked out of deprecated ActionList all the way to SelectPanel. I don't think we want folks to use the full width with SelectPanel.
One idea might be (For the sake of keeping things uncomplicated) we can merge them and immediately deprecated the old variants?
I like that
Luckily, there is no usage in dotcom and only 5 instances in internal status page, so it feels like we can deprecate/remove them for modern ActionList in SelectPanel
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/350395 |
🔴 golden-jobs completed with status |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I think the hover color is different?? It's really hard to tell
Couple of followup improvements (not blockers for this PR):
Rollout strategy
Testing & Reviewing
Merge checklist