-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(Button): render labeled
as a <button>
#597
Conversation
fix switch button to div
Current coverage is 100% (diff: 100%)@@ master #597 diff @@
====================================
Files 119 119
Lines 1915 1915
Methods 0 0
Messages 0 0
Branches 0 0
====================================
Hits 1915 1915
Misses 0 0
Partials 0 0
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, just need a test for this. See test/specs/elements/Button
. Should be a very simply copy/paste update of one of the existing tests.
Add your review
Should work in production this trick fixed the problem. |
wrapper.should.have.tagName('button') | ||
wrapper.should.have.className('labeled') | ||
wrapper.should.have.exactly(1).descendants('Icon') | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test, let's simplify it a bit. It is best to write tests that are as minimal as possible and assert one thing. This makes them stronger and more maintainable.
In this case, most of the assertions made here are covered by other existing tests, all we should need to assert is that a labeled
button renders a button
element:
it('renders as a button', () => {
shallow(<Button labeled />)
.should.have.tagName('button')
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made
labeled
as a <button>
Also, if the description of this PR were updated slightly, it will auto close the related issue on merge:
https://github.com/blog/1506-closing-issues-via-pull-requests |
Fix switch button to div
#596