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

Add .BtnGroup-parent, deprecate .BtnGroup-form #522

Merged
merged 2 commits into from
Jul 3, 2018
Merged

Add .BtnGroup-parent, deprecate .BtnGroup-form #522

merged 2 commits into from
Jul 3, 2018

Conversation

muan
Copy link
Contributor

@muan muan commented Jul 2, 2018

  • There doesn't seem to be a dev branch for me to compare against
  • For some reasons I can't push to primer repo directly anymore but I definitely still have admin access 🤔

Anyways this PR adds .BtnGroup-parent and keeps .BtnGroup-form for backwards compatibility. parent is a more generic name since this class can be used on more than just <form>. I had to use it on <details> a few times already.

Is this OK? is there more we need to do for class name deprecation?

🌹

@muan muan requested a review from a team July 2, 2018 14:08
@shawnbot
Copy link
Contributor

shawnbot commented Jul 3, 2018

I like this change a lot. It's something we noticed when attempting to re-implement the Merge pull request button from the merge box in primer-react, too.

This is definitely a breaking change, so we'd need to queue up removal for v11 in #498. In the meantime, we can call out the deprecation with @warn (as in colorizeTooltip() or .avatar-stack), update the style guide to reference the new class, then do a one-time search-and-replace to kill off any of the old instances.

Copy link
Contributor

@emplums emplums left a comment

Choose a reason for hiding this comment

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

This looks good to me! I agree with @shawnbot we should add a deprecation warning as part of this PR if possible! 💖

Regarding the dev branch, we've decided to get rid of it, and compare directly against our release branches instead. It's not in the documentation because we figured ds-core could be in charge of updating base branches, but in practice it feels a bit awkward 🤔 I'll think on that and update the docs accordingly!

@emplums emplums changed the base branch from master to release-10.8.0 July 3, 2018 17:41
@muan
Copy link
Contributor Author

muan commented Jul 3, 2018

Copy link
Contributor

@shawnbot shawnbot left a comment

Choose a reason for hiding this comment

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

Beautiful, thanks @muan! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants