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.

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

Update ButtonGroup to use Beta::Button #2013

Merged
merged 21 commits into from
May 25, 2023
Merged

Update ButtonGroup to use Beta::Button #2013

merged 21 commits into from
May 25, 2023

Conversation

langermank
Copy link
Contributor

@langermank langermank commented May 12, 2023 •
edited
Loading

Description

  • Removes deprecated Button component
  • Adds CSS for ButtonGroup class
  • Add preview for split button ActionMenu scenario (WIP)

Integration

Does this change require any updates to code in production?

Maybe, this might be kind of a breaking change? This is used 18 times in production and I'm happy to roll it out with any fixes.

Replace variant: :small with size: :small https://github.com/search?q=repo%3Agithub%2Fgithub+Primer%3A%3ABeta%3A%3AButtonGroup&type=code

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Added/updated previews

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 0afa94b

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

This PR includes changesets to release 1 package
Name Type
@primer/view-components Patch

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

@langermank langermank temporarily deployed to preview May 12, 2023 20:51 — with GitHub Actions Inactive
@github-actions github-actions bot added css Pull requests that update CSS code ruby Pull requests that update Ruby code labels May 12, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 12, 2023 •
edited
Loading

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

@langermank langermank marked this pull request as ready for review May 12, 2023 21:15
@langermank langermank requested a review from a team as a code owner May 12, 2023 21:15
@langermank langermank requested review from a team, tobiasahlin and keithamus May 12, 2023 21:15
@langermank langermank requested a review from jonrohan May 12, 2023 21:20
@langermank langermank temporarily deployed to preview May 12, 2023 21:23 — with GitHub Actions Inactive
@jonrohan
Copy link
Member

jonrohan commented May 12, 2023 •
edited by langermank
Loading

Taking a quick look the only thing it looks like we'll need to fix is variant: is used but not in too many places, I think this could be doable https://github.com/search?q=repo%3Agithub%2Fgithub+Primer%3A%3ABeta%3A%3AButtonGroup&type=code

@langermank langermank temporarily deployed to preview May 15, 2023 21:56 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to github-pages May 15, 2023 22:00 — with GitHub Actions Inactive
jonrohan
jonrohan reviewed May 15, 2023
View reviewed changes
test/components/beta/button_group_test.rb Outdated
Comment on lines 25 to 26
component.with_button(scheme: :primary) { "Primary" }
component.with_button(scheme: :danger) { "Danger" }
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to pass the scheme to ButtonGroup now

jonrohan
jonrohan reviewed May 15, 2023
View reviewed changes
app/components/primer/beta/button_group.rb
Comment on lines +43 to +44
@size = size
@scheme = scheme
Copy link
Member

@jonrohan jonrohan May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You might want to use fetch or fallback here and the Button scheme and size options

Just thinking out loud maybe not since the button will take care of that anyway. Never mind

@langermank langermank temporarily deployed to preview May 15, 2023 22:21 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview May 15, 2023 23:16 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to github-pages May 15, 2023 23:20 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to preview May 16, 2023 00:01 — with GitHub Actions Inactive
@langermank langermank temporarily deployed to github-pages May 16, 2023 00:05 — with GitHub Actions Inactive
@langermank langermank enabled auto-merge (squash) May 16, 2023 18:24
@langermank langermank disabled auto-merge May 16, 2023 18:25
@langermank
Copy link
Contributor Author

This is ready to merge, but I'd like to be available to help roll it out and make dotcom changes. I will be out May 22-26.

jonrohan
jonrohan approved these changes May 23, 2023
View reviewed changes
Copy link
Member

@jonrohan jonrohan left a comment

Choose a reason for hiding this comment

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

The older release is out, 🚀

@langermank langermank merged commit b7207d2 into main May 25, 2023
@langermank langermank deleted the button-group-beta-button branch May 25, 2023 19:28
@primer-css primer-css mentioned this pull request May 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonrohan jonrohan jonrohan approved these changes

@camertron camertron camertron approved these changes

@tobiasahlin tobiasahlin Awaiting requested review from tobiasahlin tobiasahlin is a code owner automatically assigned from primer/css-reviewers

@keithamus keithamus Awaiting requested review from keithamus

Assignees
No one assigned
Labels
css Pull requests that update CSS code patch release ruby Pull requests that update Ruby code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@langermank @jonrohan @camertron