Skip to content

Add optional footer prop to SelectPanel #4353

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.

Sign up for GitHub

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

Jump to bottom
Merged
siddharthkp merged 10 commits into from
Mar 11, 2024
Merged

Add optional footer prop to SelectPanel #4353

siddharthkp merged 10 commits into from
Mar 11, 2024

Conversation

@anleac
Copy link

@anleac anleac commented Mar 5, 2024 •
edited
Loading

Issue - https://github.com/github/issues/issues/8775

This PR now adds an optional footer that will be rendered, sticky, at the bottom of the SelectPanel if provided. This could be useful for adding in additional options to be present.

Showing the sticky footer on a multiselect panel

Changelog

New

New prop, footer, to SelectPanel.

Changed

N/A.

Removed

N/A.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Testing via the storybook should be sufficient.

Merge checklist

@anleac anleac requested review from a team and pksjce March 5, 2024 12:23
@changeset-bot
Copy link

changeset-bot bot commented Mar 5, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: c495eaa

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2024 •
edited
Loading

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.07 KB (-0.04% 🔽)
packages/react/dist/browser.umd.js 89.29 KB (-0.08% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4353 March 5, 2024 12:27 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4353 March 5, 2024 22:54 Inactive
@pksjce
Copy link
Contributor

pksjce commented Mar 5, 2024

Since this is the old SelectPanel and does not affect existing usages in anyway, I'm fine to approve this.
But the focus does look sloppy. I guess if @anleac is ok with that and we are definitely phasing this component out asap, it should be ok?

Screenshot 2024-03-05 at 2 57 20 pm

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2024

Uh oh! @pksjce, the image you shared is missing helpful alt text. Check #4353 (comment).

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

@pksjce pksjce requested a review from siddharthkp March 5, 2024 23:44
@github-actions github-actions bot temporarily deployed to storybook-preview-4353 March 6, 2024 07:36 Inactive
@siddharthkp
Copy link
Member

siddharthkp commented Mar 6, 2024 •
edited
Loading

+1 the styles seem a bit sloppy here:

focus is bleeding out

Can we also update the story to represent the use case we are adding a footer for. I'm guessing "Close" isn't something we want to promote?

If we want, we could take inspiration from SelectPanel v2:

Storybook: https://primer.style/react/storybook/?path=/story/drafts-components-selectpanel-features--instant-selection-variant

footer button in SelectPanel v2

@anleac anleac requested a review from a team as a code owner March 7, 2024 14:08
@anleac anleac requested a review from broccolinisoup March 7, 2024 14:08
@anleac
Copy link
Author

anleac commented Mar 7, 2024

thanks both @siddharthkp and @pksjce ! I update the examples to better reflect the use-case, let's also do a follow up for the focus state later.

@github-actions github-actions bot temporarily deployed to storybook-preview-4353 March 7, 2024 14:12 Inactive
@broccolinisoup broccolinisoup removed their request for review March 11, 2024 00:40
@siddharthkp
Copy link
Member

siddharthkp commented Mar 11, 2024 •
edited
Loading

Tiny update: Moved space inside the footer instead of story + Updated changelog

siddharthkp
siddharthkp approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@siddharthkp siddharthkp 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 now!

@siddharthkp siddharthkp added this pull request to the merge queue Mar 11, 2024
Merged via the queue into main with commit 2c0086e Mar 11, 2024
@siddharthkp siddharthkp deleted the panel-update branch March 11, 2024 14:49
@primer primer bot mentioned this pull request Mar 11, 2024
Merged
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
@pksjce @siddharthkp @lukasoppermann
Add optional footer prop to SelectPanel (#4353)
d05379a 
* Spike on adding a footer

* Update changelog

* Storybook update

* Update styling

* move margin to internal padding space

* Update bright-foxes-hunt.md

* Update bright-foxes-hunt.md

---------

Co-authored-by: Pavithra Kodmad <[email protected]>
Co-authored-by: Siddharth Kshetrapal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@siddharthkp siddharthkp siddharthkp approved these changes

+1 more reviewer

@pksjce pksjce pksjce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anleac @pksjce @siddharthkp