Skip to content
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

SelectPanel2: Instant selection variant #3783

Merged
merged 8 commits into from
Oct 24, 2023
Merged

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Oct 2, 2023

Context:

<SelectPanel selectionVariant="instant">
  ...
</SelectPanel>
  1. renders ActionList with selectionVariant=single
  2. does not render primary footer actions
  3. Selecting an item calls onSubmit and closes the menu instantly
instant-selectpanel.mov

@siddharthkp siddharthkp added the skip changeset This change does not need a changelog label Oct 2, 2023
@siddharthkp siddharthkp requested review from broccolinisoup and a team October 2, 2023 13:50
@siddharthkp siddharthkp self-assigned this Oct 2, 2023
@changeset-bot

This comment was marked as resolved.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.62 KB (0%)
dist/browser.umd.js 105.2 KB (0%)


if (hidePrimaryActions && !props.children) {
// nothing to render
// todo: we can inform them the developer footer will render nothing
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not throwing validation errors yet. Will create a PR for all validations errors together

@siddharthkp siddharthkp temporarily deployed to github-pages October 2, 2023 13:56 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3783 October 2, 2023 13:56 Inactive
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me ⭐ Just left two comments/questions

Comment on lines +279 to +288
{hidePrimaryActions ? null : (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally optional - I usually find the syntax below more readable but feel free to ignore if it doesn't resonate with you 🤗

Suggested change
{hidePrimaryActions ? null : (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}
{!hidePrimaryActions && (
<Box sx={{display: 'flex', gap: 2}}>
<Button size="small" type="button" onClick={() => onCancel()}>
Cancel
</Button>
<Button size="small" type="submit" variant="primary">
Save
</Button>
</Box>
)}

Copy link
Member Author

@siddharthkp siddharthkp Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but feel free to ignore if it doesn't resonate with you 🤗

Firstly, thank you! I appreciate that!

because I feel the opposite 😅

I find double negatives like !hide confusing. don't hide = so show. There's something really clean about hide → return null

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 Fair enough!!

src/drafts/SelectPanel2/SelectPanel.tsx Show resolved Hide resolved
pksjce

This comment was marked as resolved.

</Box>
)
}
SelectPanel.Footer = SelectPanelFooter

// @ts-ignore todo
SelectPanel.SecondaryButton = props => {
return <Button {...props} size="small" type="button" />
return <Button {...props} size="small" type="button" block />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering why this Button needs to be block type?

Copy link
Member Author

@siddharthkp siddharthkp Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question!

So, for most cases of SelectPanel you'd see a Save and Cancel button on the right, but then also an optional secondary button on the left:

select panel with save, cancel and a secondary action in the footer

In the case of instant variant, there is no Save and Cancel. So, we'd like the secondary button to take the full width of the footer so it looks less awkward:

Without block With block
select panel with just a secondary action in the footer, taking the full width of the footer select panel with just a secondary action in the footer, awkwardly sitting in the left corner

Now because the footer has 2 divs inside of it - left buttons container and right buttons container, it's the left container that "flex-grows" based on variant. Inside the left container, secondary button can always take the full width (block) available to it.

left container with flexGrow: 0 left container with flexGrow: 1
image image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see! thanks for the explainer!

@siddharthkp
Copy link
Member Author

While I'm able to see the story for Instant selection variant, the other stories for SelectPanel2 seem to be erroring out

Thanks @pksjce for catching that! Fixed now!

@siddharthkp siddharthkp added this pull request to the merge queue Oct 24, 2023
Merged via the queue into main with commit efa4650 Oct 24, 2023
29 checks passed
@siddharthkp siddharthkp deleted the drafts-selectpanel-instant branch October 24, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants