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

feat(button): add tertiary and ghost danger buttons #7087

Conversation

AlexanderLyon
Copy link
Contributor

@AlexanderLyon AlexanderLyon commented Oct 20, 2020

Closes #4945
Closes #5610

Changelog

New

  • Adds tertiary and ghost danger button variant styles
    • These are implemented as new "kinds" (i.e. danger-tertiary and danger-ghost respectively)
  • Adds tests to Button-test.js to verify these new variants render successfully
  • Added new danger button variants to Storybook

Testing / Reviewing

  • See "Danger Ghost" and "Danger Tertiary" in Storybook to verify correct styles

@AlexanderLyon AlexanderLyon requested a review from a team as a code owner October 20, 2020 03:49
@netlify
Copy link

netlify bot commented Oct 20, 2020

Deploy preview for carbon-elements ready!

Built with commit 4292b0f

https://deploy-preview-7087--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 20, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 4292b0f

https://deploy-preview-7087--carbon-components-react.netlify.app

@emyarod emyarod requested review from a team and johnbister and removed request for a team October 20, 2020 20:10
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

thanks for opening this PR, I requested a review from our design team. in the meantime, you'll need to resolve the SassDoc conflict (probably by rerunning the build npm script for the components package)

@AlexanderLyon
Copy link
Contributor Author

@emyarod the merge conflict has been resolved

@AlexanderLyon
Copy link
Contributor Author

Do you think it would make more sense to show all danger button variations under Button -> Danger in Storybook, rather than each separately as it is now (ex. Button -> Danger, Button -> Danger Tertiary, and Button -> Danger Ghost)?

@johnbister johnbister requested a review from aagonzales October 22, 2020 17:10
@aagonzales
Copy link
Member

@AlexanderLyon Yes I think it would be good to show them all under one Danger button story.

@AlexanderLyon
Copy link
Contributor Author

@aagonzales sure thing - I've updated storybook to display all 3 danger variants under the same story

@johnbister johnbister removed their request for review October 26, 2020 15:19
Copy link
Member

@aagonzales aagonzales 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! Thanks for contributing!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

since the Danger Button story now has multiple variants, should we label them accordingly?

<>
<Button kind="danger">Button</Button>
&nbsp;
<Button kind="danger-tertiary">Button</Button>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Button kind="danger-tertiary">Button</Button>
<Button kind="danger-tertiary">Tertiary Danger Button</Button>

&nbsp;
<Button kind="danger-tertiary">Button</Button>
&nbsp;
<Button kind="danger-ghost">Button</Button>
Copy link
Member

@emyarod emyarod Oct 26, 2020

Choose a reason for hiding this comment

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

Suggested change
<Button kind="danger-ghost">Button</Button>
<Button kind="danger-ghost">Ghost Danger Button</Button>

@AlexanderLyon
Copy link
Contributor Author

Good point @emyarod, this has been updated in 7ffd560!

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

thanks for the update, looks good to me

@@ -4447,9 +4449,13 @@ Define theme variables from a map of tokens
--#{$custom-property-prefix}-overlay-01,
map-get($theme, 'overlay-01')
) !global;
$danger: var(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change

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.

[danger button] add tertiary and ghost variants Need danger button alternatives
4 participants