Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

breaking(Button): replace type prop with secondary and primary #419

Merged
merged 6 commits into from
Nov 2, 2018

Conversation

layershifter
Copy link
Member

Fixes #374, please refer there to get more details.

@codecov
Copy link

codecov bot commented Nov 2, 2018

Codecov Report

Merging #419 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
- Coverage   91.77%   91.76%   -0.01%     
==========================================
  Files          41       41              
  Lines        1325     1324       -1     
  Branches      169      168       -1     
==========================================
- Hits         1216     1215       -1     
  Misses        105      105              
  Partials        4        4
Impacted Files Coverage Δ
src/components/Button/Button.tsx 95.74% <100%> (-0.09%) ⬇️

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 5bb41a4...2affefa. Read the comment docs.

Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

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

pls take a look at comments

if (onClick) {
onClick(e, this.props)
}
_.invoke(this.props, 'onClick', e, this.props)
Copy link
Member Author

Choose a reason for hiding this comment

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

Simplify with invoke()

@layershifter layershifter force-pushed the breaking/button-type branch 2 times, most recently from d7739a3 to a44b20c Compare November 2, 2018 11:53
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

The added test is failing.

If we decided to remove type for Button we should do the same for Menu and Divider in the same release.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Nov 2, 2018
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

Don't forget to update CHANGELOG.md

CHANGELOG.md Outdated
@@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Fixes
- Fix endMedia to not be removed from DOM on mouseleave for `ListItem` @musingh1 ([#278](https://github.com/stardust-ui/react/pull/278))
- Replace the `type` prop with `secondary` and `primary` for `Button` @layershifter ([#419](https://github.com/stardust-ui/react/pull/419))
Copy link
Member

Choose a reason for hiding this comment

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

This should go to BREAKING CHANGES!

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, done

@bmdalex
Copy link
Collaborator

bmdalex commented Nov 2, 2018

agreed with @miroslavstastny
@layershifter
do we have an issue/ticket to track the other type props (for Menu and Divider )?
we should have PRs for these soon, i can help with that
fyi @alinais

@layershifter layershifter merged commit f898e99 into master Nov 2, 2018
@layershifter layershifter deleted the breaking/button-type branch November 2, 2018 13:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants