-
Notifications
You must be signed in to change notification settings - Fork 4k
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(Header): Add subheader prop #476
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import React from 'react' | ||
import { Header } from 'stardust' | ||
|
||
const HeaderSubheaderPropExample = () => ( | ||
<Header as='h2' content='Account Settings' subheader='Manage your account settings and set email preferences' /> | ||
) | ||
|
||
export default HeaderSubheaderPropExample |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ import HeaderContent from './HeaderContent' | |
function Header(props) { | ||
const { | ||
color, content, dividing, block, attached, floated, inverted, disabled, sub, size, textAlign, | ||
icon, image, children, className, | ||
icon, image, children, className, subheader, | ||
} = props | ||
|
||
const classes = cx( | ||
|
@@ -47,22 +47,29 @@ function Header(props) { | |
|
||
if (icon && typeof icon !== 'boolean') { | ||
return ( | ||
<ElementType className={classes} {...rest}> | ||
<ElementType {...rest} className={classes}> | ||
{createIcon(icon)} | ||
{content && <HeaderContent>{content}</HeaderContent>} | ||
{subheader && <HeaderSubheader content={subheader} />} | ||
</ElementType> | ||
) | ||
} | ||
|
||
if (image) { | ||
return ( | ||
<ElementType className={classes} {...rest}> | ||
<ElementType {...rest} className={classes}> | ||
{createImage(image)} {content} | ||
{subheader && <HeaderSubheader content={subheader} />} | ||
</ElementType> | ||
) | ||
} | ||
|
||
return <ElementType {...rest} className={classes}>{children || content}</ElementType> | ||
return ( | ||
<ElementType {...rest} className={classes}> | ||
{children || content} | ||
{subheader && <HeaderSubheader content={subheader} />} | ||
</ElementType> | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, the Header will render children with shorthand even though there are prop type warnings. While we're here, let's split this return with a conditional return to only render children if provided. The goal in doing this in all components is to enforce proper usage (not rendering shorthand when children are present): if (children) {
return (
<ElementType {...rest} className={classes}>
{children}
</ElementType>
)
}
return (
<ElementType {...rest} className={classes}>
{content}
{subheader && <HeaderSubheader content={subheader} />}
</ElementType>
) |
||
} | ||
|
||
Header._meta = { | ||
|
@@ -156,6 +163,12 @@ Header.propTypes = { | |
/** Content headings are sized with em and are based on the font-size of their container. */ | ||
size: PropTypes.oneOf(Header._meta.props.size), | ||
|
||
/** Shorthand for the Header.Subheader component. Mutually exclusive with children */ | ||
subheader: customPropTypes.every([ | ||
customPropTypes.disallow(['children']), | ||
PropTypes.string, | ||
]), | ||
|
||
/** Align header content */ | ||
textAlign: PropTypes.oneOf(Header._meta.props.textAlign), | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,22 @@ | ||
import React, { PropTypes } from 'react' | ||
import cx from 'classnames' | ||
import { getElementType, getUnhandledProps, META } from '../../lib' | ||
import React, { PropTypes } from 'react' | ||
|
||
import { | ||
customPropTypes, | ||
getElementType, | ||
getUnhandledProps, | ||
META, | ||
} from '../../lib' | ||
|
||
function HeaderSubheader(props) { | ||
const { children, className } = props | ||
const { children, className, content } = props | ||
const classes = cx('sub header', className) | ||
const rest = getUnhandledProps(HeaderSubheader, props) | ||
const ElementType = getElementType(HeaderSubheader, props) | ||
|
||
return ( | ||
<ElementType className={classes} {...rest}> | ||
{children} | ||
{children || content} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case, having a single return is OK because either children or content will be rendered. But, never will children and shorthand be rendered. 👍 |
||
</ElementType> | ||
) | ||
} | ||
|
@@ -28,11 +34,20 @@ HeaderSubheader.propTypes = { | |
PropTypes.func, | ||
]), | ||
|
||
/** Primary content of the HeaderSubheader */ | ||
children: PropTypes.node, | ||
/** Primary content of the HeaderSubheader. Mutually exclusive with content */ | ||
children: customPropTypes.every([ | ||
customPropTypes.disallow(['content']), | ||
PropTypes.node, | ||
]), | ||
|
||
/** Classes to add to the subheader className. */ | ||
className: PropTypes.string, | ||
|
||
/** Shorthand for primary content. Mutually exclusive with children */ | ||
content: customPropTypes.every([ | ||
customPropTypes.disallow(['children']), | ||
PropTypes.string, | ||
]), | ||
} | ||
|
||
export default HeaderSubheader |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,6 @@ | ||
import faker from 'faker' | ||
import React from 'react' | ||
|
||
import Header from 'src/elements/Header/Header' | ||
import HeaderContent from 'src/elements/Header/HeaderContent' | ||
import HeaderSubheader from 'src/elements/Header/HeaderSubheader' | ||
|
@@ -47,7 +49,7 @@ describe('Header', () => { | |
describe('content', () => { | ||
it('adds child text', () => { | ||
shallow(<Header content='foo' />) | ||
.should.have.prop('children', 'foo') | ||
.should.contain.text('foo') | ||
}) | ||
it('adds child text when there is an image', () => { | ||
shallow(<Header content='foo' image='foo.png' />) | ||
|
@@ -65,4 +67,28 @@ describe('Header', () => { | |
wrapper.should.not.have.descendants('HeaderContent') | ||
}) | ||
}) | ||
|
||
describe('subheader', () => { | ||
it('adds HeaderSubheader as child', () => { | ||
const text = faker.hacker.phrase() | ||
|
||
shallow(<Header subheader={text} />) | ||
.find('HeaderSubheader') | ||
.should.have.prop('content', text) | ||
}) | ||
it('adds HeaderSubheader as child when given a name to icon prop', () => { | ||
const text = faker.hacker.phrase() | ||
|
||
shallow(<Header icon='user' subheader={text} />) | ||
.find('HeaderSubheader') | ||
.should.have.prop('content', text) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for shallow rendering and only testing this prop was passed, instead of testing the Subheader's handling of the prop in the Header test 👍 This is a great test. |
||
}) | ||
it('adds HeaderSubheader as child when there is an image', () => { | ||
const text = faker.hacker.phrase() | ||
|
||
shallow(<Header image='foo.png' subheader={text} />) | ||
.find('HeaderSubheader') | ||
.should.have.prop('content', text) | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,19 @@ | ||
import faker from 'faker' | ||
import React from 'react' | ||
|
||
import HeaderSubheader from 'src/elements/Header/HeaderSubheader' | ||
import * as common from 'test/specs/commonTests' | ||
|
||
describe('HeaderSubheader', () => { | ||
common.isConformant(HeaderSubheader) | ||
common.rendersChildren(HeaderSubheader) | ||
|
||
describe('content', () => { | ||
it('renders text', () => { | ||
const text = faker.hacker.phrase() | ||
|
||
shallow(<HeaderSubheader content={text} />) | ||
.should.contain.text(text) | ||
}) | ||
}) | ||
}) |
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.
Seems, I missed test-case there.