-
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(Header): Fix image
prop
#545
Conversation
@@ -53,7 +54,7 @@ function Header(props) { | |||
) | |||
} | |||
|
|||
if (image || icon && typeof icon !== 'boolean') { | |||
if ((image && typeof image !== 'boolean') || (icon && typeof icon !== 'boolean')) { |
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.
Impoved condition.
.should.not.have.className('icon') | ||
it('does not add an Image when true', () => { | ||
shallow(<Header image />) | ||
.should.not.have.descendants('Image') |
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.
Added test.
useKeyOnly(inverted, 'inverted'), | ||
useKeyOnly(disabled, 'disabled'), | ||
useKeyOnly(sub, 'sub'), |
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.
Sorted props.
Current coverage is 98.86% (diff: 100%)@@ master #545 diff @@
==========================================
Files 108 108
Lines 1764 1764
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 1744 1744
Misses 20 20
Partials 0 0
|
…into fix/header-prop Conflicts: test/specs/elements/Header/Header-test.js
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.
Just one comment, then good to merge.
@@ -69,7 +80,14 @@ describe('Header', () => { | |||
}) | |||
|
|||
describe('subheader', () => { | |||
it('adds HeaderSubheader as child when there is an icon', () => { | |||
it('adds HeaderSubheader as child', () => { |
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.
This one should be covered by implementsShorthandProp()
for subheader
. Should be able to remove it without a change in coverage.
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.
Looks like issue with merge, now okay.
…into fix/header-prop Conflicts: test/specs/elements/Header/Header-test.js
Released in |
Fixes #542.