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 separator in button sets #6608

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Aug 4, 2020

Ref #5638

This PR adds a separator between buttons (in button sets only) with a new $button-separator token

Changelog

New

  • ButtonSet wrapper component
  • $button-separator token for button sets
  • support for vertically stacked button set orientation

Changed

  • refactor styles and selectors to allow for sets to contain more than two buttons

Testing / Reviewing

Confirm the separators appear correct and for button sets only

@netlify
Copy link

netlify bot commented Aug 4, 2020

Deploy preview for carbon-elements ready!

Built with commit 2eb2e97

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

@netlify
Copy link

netlify bot commented Aug 4, 2020

Deploy preview for carbon-components-react ready!

Built with commit 2eb2e97

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

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

We can get rid of the gray 20 rule at the end of the last button. Otherwise everything looks great!

Screen Shot 2020-08-04 at 2 20 18 PM

@emyarod
Copy link
Member Author

emyarod commented Aug 4, 2020

would we want to just create a ButtonSet wrapper component to help create button sets? instead of relying on users adding the bx--btn-set class to the container div?

@joshblack
Copy link
Contributor

joshblack commented Aug 5, 2020

@emyarod I think that makes sense, maybe a ButtonGroup component? And folks could specify direction (row, column), or I guess pass in a stacked prop?

Group comes up only because that's what I remember seeing in docs, let me know if sets is a better term for these (especially since we have that class already in code)

@emyarod
Copy link
Member Author

emyarod commented Aug 5, 2020

yeah exactly I was thinking of something like ButtonSet since that's the name of the story and the class names in code

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Secondary button in button sets: The focus is slightly off and should follow this spec.

button 2


Primary button in dark themes for button sets and button stories: For focus we should be using a transparent 1px space instead of gray, but I am noticing this may be a Button bug in general.
button1

@emyarod
Copy link
Member Author

emyarod commented Aug 7, 2020

the focus issue should be resolved now, and I will problems that are inherited from the base button styles separately since they are not directly related to the current PR!

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

When the Secondary button receives focus, we need the focus to be on top of the rule separator between the buttons. Right now it looks like the focus is getting cut off because it is behind the rule.
Artboard


For the stacked set of buttons, we do not need to include the top rule on the secondary button.

Screen Shot 2020-08-07 at 3 55 47 PM

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

-The stacked secondary button on focus still is getting overlapped by the rule separator.
Screen Shot 2020-08-10 at 11 41 19 AM


-The token being using for the rule separator needs to be Gray 20 #e0e0e0 for the White theme and and Gray 10 theme. It looks like it is using Gray 100 right now.
dark line

Copy link
Member

@laurenmrice laurenmrice 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 🙌🏻

@kodiakhq kodiakhq bot merged commit aca2405 into carbon-design-system:master Aug 13, 2020
@emyarod emyarod deleted the 5638-button-contrast-a11y branch August 18, 2020 19:51
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.

5 participants