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 buttons shorthand to ButtonGroup #2361

Merged
merged 4 commits into from
Dec 10, 2017

Conversation

layershifter
Copy link
Member

Fixes #2310.

This PRs adds new shorthand and updates existing docs to latest style.

@@ -24,8 +26,6 @@ describe('ButtonGroup', () => {
common.propKeyOnlyToClassName(ButtonGroup, 'negative')
common.propKeyOnlyToClassName(ButtonGroup, 'positive')
common.propKeyOnlyToClassName(ButtonGroup, 'primary')
common.propKeyOnlyToClassName(ButtonGroup, 'primary')
common.propKeyOnlyToClassName(ButtonGroup, 'secondary')
Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicates 😺

Copy link
Member

Choose a reason for hiding this comment

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

😱

@@ -34,4 +34,15 @@ describe('ButtonGroup', () => {

common.propValueOnlyToClassName(ButtonGroup, 'color', SUI.COLORS)
common.propValueOnlyToClassName(ButtonGroup, 'size', SUI.SIZES)

describe('buttons', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

@levithomason Should we go with buttons or items prop?

Copy link
Member

Choose a reason for hiding this comment

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

I think buttons for consistency. The name should follow the sub-component or child components in this case:

<Breadcrumb sections={...} />
<Feed events={...} />
<Modal actions={...} />
<Button.Group buttons={...} />

The exception is <Card.Group items={...} itemsPerRow={...} /> which was an early exploratory API. However, this will be replaced with standardized width props, #406. It should be: <Card.Group cards={...} widths={...} />.

@codecov-io
Copy link

codecov-io commented Dec 6, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@f9348a2). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2361   +/-   ##
=========================================
  Coverage          ?   99.73%           
=========================================
  Files             ?      152           
  Lines             ?     2664           
  Branches          ?        0           
=========================================
  Hits              ?     2657           
  Misses            ?        7           
  Partials          ?        0
Impacted Files Coverage Δ
src/elements/Button/ButtonGroup.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9348a2...b12ecb4. Read the comment docs.

@@ -93,6 +104,9 @@ ButtonGroup.propTypes = {
/** Groups can be less pronounced. */
basic: PropTypes.bool,

/** Shorthand array of props for Button. */
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't necessarily have to be props, it can be any shorthand value.

@levithomason
Copy link
Member

LGTM, I've made one minor update to the doc block for buttons. This is ready to merge once CI passes.

@layershifter layershifter merged commit 9e2bf27 into master Dec 10, 2017
@layershifter layershifter deleted the feat/button-shorthand branch December 10, 2017 20:00
@levithomason
Copy link
Member

Released in [email protected]

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