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

Replace deprecated Button in Dialog #4049

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

TylerJDev
Copy link
Contributor

@TylerJDev TylerJDev commented Dec 11, 2023

Closes https://github.com/github/primer/issues/2470, #3062

Changelog

Changed

Replaces deprecated Button component in Dialog.tsx with newest version.

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

For a successful rollout in github, we'll need to adjust some items that rely on the deprecated version of Button. The changes needed can be viewed in the integration test PR.

Testing & Reviewing

Ensure that a focus indicator is present when focused on any of the dialog's footer buttons.

Dialog default Storybook example.

Merge checklist

Copy link

changeset-bot bot commented Dec 11, 2023

🦋 Changeset detected

Latest commit: 61df519

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

Copy link
Contributor

github-actions bot commented Dec 11, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.71 KB (-0.8% 🔽)
dist/browser.umd.js 105.27 KB (-0.82% 🔽)

@github-actions github-actions bot temporarily deployed to storybook-preview-4049 December 11, 2023 21:24 Inactive
@TylerJDev TylerJDev marked this pull request as ready for review December 12, 2023 21:35
@TylerJDev TylerJDev requested review from a team and mperrotti December 12, 2023 21:35
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

These changes look great to me and the upstream PR makes it seem like we only need to update tests 💯 I'll also request @broccolinisoup who I think may also have some helpful context for migrating this work if I remember right (I think she had helped an outside contributor trying to do this in the past and I wasn't sure what the hiccups ended up being there)

@broccolinisoup
Copy link
Member

These changes look great to me and the upstream PR makes it seem like we only need to update tests 💯 I'll also request @broccolinisoup who I think may also have some helpful context for migrating this work if I remember right (I think she had helped an outside contributor trying to do this in the past and I wasn't sure what the hiccups ended up being there)

Thanks @joshblack for the ping, I appreciate it! We chatted about it in our call with Tyler and the reason that we reverted this PR is addressed in the integration PR 🤗 And the solution is approved by the code owners. Seems like we are good to go!

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 great! Thank you so much for fixing that and providing the integration fixes as well ✨

@TylerJDev TylerJDev added this pull request to the merge queue Dec 19, 2023
Merged via the queue into main with commit f0d38bc Dec 19, 2023
29 checks passed
@TylerJDev TylerJDev deleted the tylerjdev/replace-deprecated-buttons-dialog branch December 19, 2023 14:33
@primer primer bot mentioned this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants