-
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
Add Breadcrumb component #321
Changes from 3 commits
f28ba81
cc969f1
a87bb2e
6eff245
3893a03
c01c4a6
23b3df4
80a2803
f3864ae
c2cbba0
ffc09a5
43deaf1
3d88988
c91d0d9
6fa9e5a
6937755
77baebb
25eb1bb
92c13d2
a44fb59
82b04fa
a9ce2e3
a299332
32b53d4
a6f0654
9edb4a0
ba14592
0f5235b
9398fa5
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,48 @@ | ||
import cx from 'classnames' | ||
import React, { PropTypes } from 'react' | ||
import META from 'src/utils/Meta' | ||
import { getUnhandledProps } from '../../utils/propUtils' | ||
import * as sui from '../../utils/semanticUtils' | ||
import BreadcrumbSection from './BreadcrumbSection' | ||
|
||
function Breadcrumb(props) { | ||
const { | ||
children, | ||
size, | ||
} = props | ||
|
||
const classes = cx( | ||
'ui', | ||
size, | ||
'breadcrumb' | ||
) | ||
|
||
const rest = getUnhandledProps(Breadcrumb, props) | ||
|
||
return ( | ||
<div className={classes} {...rest}> | ||
{children} | ||
</div> | ||
) | ||
} | ||
|
||
Breadcrumb._meta = { | ||
library: META.library.semanticUI, | ||
name: 'Breadcrumb', | ||
type: META.type.collection, | ||
props: { | ||
size: sui.sizes, | ||
}, | ||
} | ||
|
||
Breadcrumb.propTypes = { | ||
/** Primary content of the Breadcrumb */ | ||
children: PropTypes.node, | ||
|
||
/** Size of Breadcrumb */ | ||
size: PropTypes.oneOf(Breadcrumb._meta.props.size), | ||
} | ||
|
||
Breadcrumb.Section = BreadcrumbSection | ||
|
||
export default Breadcrumb |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import * as _ from 'lodash' | ||
import React, { PropTypes } from 'react' | ||
import cx from 'classnames' | ||
import META from '../../utils/Meta' | ||
|
||
/** | ||
* A section sub-component for Breadcrumb component. | ||
* | ||
* @see Breadcrumb | ||
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. The Subcomponents however do not yet show up on the doc site. Let's remove the Side note, we will likely expose subcomponents in the future, but it will be automated. We can do this since the _meta property already defines the parent component. |
||
*/ | ||
function BreadcrumbSection(props) { | ||
const { | ||
active, children, className, link, href, onClick, ...rest, | ||
} = props | ||
|
||
/** | ||
* Handles onClick event. If onClick is not defined, does nothing. | ||
* */ | ||
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. Reserve doc block comments ( |
||
const handleClick = (e) => { | ||
if (!_.isUndefined(e)) e.preventDefault() | ||
if (!_.isFunction(onClick)) return | ||
|
||
onClick() | ||
} | ||
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. Few notes on event handlers.
The refactored const handleClick = (e) => {
if (onClick) onClick(e)
} Since now all this function does is pass the event from one function to another, it can be removed entirely. We can just spread the There are times that we want to first capture a click handler. This would be when we want to pass some additional arguments after the event argument. |
||
|
||
const classes = cx( | ||
active && 'active', | ||
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. This is arguably cleaner than the using the util we have for this, and we may do away with this util in the future. However, for consistency let's use the util here. We can refactor later if we do decide to remove this simple util: import { useKeyOnly } from '../../utils/propUtils'
// ...
const classes = cx(
useKeyOnly(active, 'active')
) |
||
className, | ||
'section', | ||
) | ||
|
||
// Handle link and onClick props. | ||
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. I think the code is clear enough here, let's remove the comment and examples for link/onClick, href, and the default variant. |
||
// | ||
// <Breadcrumb.Section link>Home</Breadcrumb.Section> | ||
// <Breadcrumb.Section onClick={this.handleClick}>Home</Breadcrumb.Section> | ||
|
||
if (link || onClick) { | ||
return ( | ||
<a {...rest} className={classes} href='#' onClick={handleClick}> | ||
{children} | ||
</a> | ||
) | ||
} | ||
|
||
// Handle href prop. | ||
// | ||
// <Breadcrumb.Section href='http://google.com'>Home</Breadcrumb.Section> | ||
|
||
if (href) { | ||
return ( | ||
<a {...rest} className={classes} href={href} onClick={handleClick}> | ||
{children} | ||
</a> | ||
) | ||
} | ||
|
||
// Default variant. | ||
// | ||
// <Breadcrumb.Section>Home</Breadcrumb.Section> | ||
|
||
return ( | ||
<div {...rest} className={classes} onClick={handleClick}> | ||
{children} | ||
</div> | ||
) | ||
} | ||
|
||
BreadcrumbSection._meta = { | ||
library: META.library.semanticUI, | ||
name: 'BreadcrumbSection', | ||
type: META.type.collection, | ||
parent: 'Breadcrumb', | ||
} | ||
|
||
BreadcrumbSection.propTypes = { | ||
/** Optional props that adds class active */ | ||
active: PropTypes.bool, | ||
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. PropTypes are self documented as optional/required via the We tend to try to use the SUI doc descriptions for features as well. In this case, the SUI doc description is not too helpful: Borrowing from DropdownItem, I suggest updating this description to: /** Style as the currently active section. */ |
||
|
||
/** Primary content of the Breadcrumb.Section */ | ||
children: PropTypes.node, | ||
|
||
/** Additional classes added to the element. */ | ||
className: PropTypes.string, | ||
|
||
/** Makes element link (<a>) instead of text (<div>) */ | ||
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. Suggested edit: -Makes element link (<a>) instead of text (<div>)
+Render as an `a` tag instead of a `div`. |
||
link: PropTypes.bool, | ||
|
||
/** Makes element link (<a>) instead of text (<div>) and adds href attribute. */ | ||
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. Suggested edit: -Makes element link (<a>) instead of text (<div>) and adds href attribute.
+Render as an `a` tag instead of a `div` and adds the href attribute. |
||
href: PropTypes.string, | ||
|
||
/** Makes element link (<a>) instead of text (<div>) and adds onClick event. */ | ||
onClick: PropTypes.func, | ||
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. I believe we can do away with this prop entirely. Since props are spread, it will be spread on the component and still work. 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. Can you tell us more? If I remove |
||
} | ||
|
||
export default BreadcrumbSection |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import React from 'react' | ||
|
||
import Breadcrumb from 'src/collections/Breadcrumb/Breadcrumb' | ||
import * as common from 'test/specs/commonTests' | ||
|
||
describe('Breadcrumb', () => { | ||
common.isConformant(Breadcrumb) | ||
common.hasUIClassName(Breadcrumb) | ||
common.rendersChildren(Breadcrumb) | ||
|
||
it('renders a <div /> element', () => { | ||
shallow(<Breadcrumb />) | ||
.should.have.tagName('div') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
import React from 'react' | ||
import Breadcrumb from 'src/collections/Breadcrumb/Breadcrumb' | ||
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. Let's import the actual component here so we don't have a dependency on how it is exposed to the user. import BreadcrumbSection from 'src/collections/Breadcrumb/BreadcrumbSection' |
||
import * as common from 'test/specs/commonTests' | ||
import sandbox from 'test/utils/Sandbox-util' | ||
|
||
describe('BreadcrumbSection', () => { | ||
common.isConformant(Breadcrumb.Section) | ||
common.rendersChildren(Breadcrumb.Section) | ||
|
||
describe('active', () => { | ||
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. For props that convert to classNames, we have |
||
it('is not by default', () => { | ||
const section = shallow(<Breadcrumb.Section>Home</Breadcrumb.Section>) | ||
|
||
section.should.not.have.className('active') | ||
section.should.have.tagName('div') | ||
}) | ||
|
||
it('is should be active by prop', () => { | ||
const section = shallow(<Breadcrumb.Section active>Home</Breadcrumb.Section>) | ||
|
||
section.should.have.className('active') | ||
section.should.have.tagName('div') | ||
}) | ||
}) | ||
|
||
describe('link prop', () => { | ||
it('is not by default', () => { | ||
shallow(<Breadcrumb.Section>Home</Breadcrumb.Section>) | ||
.should.have.tagName('div') | ||
}) | ||
|
||
it('is should be active by prop', () => { | ||
const section = shallow(<Breadcrumb.Section link>Home</Breadcrumb.Section>) | ||
|
||
section.should.have.tagName('a') | ||
section.should.have.attr('href').and.equal('#') | ||
}) | ||
}) | ||
|
||
describe('href prop', () => { | ||
it('is not by default', () => { | ||
const section = shallow(<Breadcrumb.Section>Home</Breadcrumb.Section>) | ||
|
||
section.should.have.tagName('div') | ||
section.should.not.have.attr('href') | ||
}) | ||
|
||
it('is should have attr when prop', () => { | ||
const section = shallow(<Breadcrumb.Section href='http://google.com'>Home</Breadcrumb.Section>) | ||
|
||
section.should.have.tagName('a') | ||
section.should.have.attr('href').and.equal('http://google.com') | ||
}) | ||
}) | ||
|
||
describe('onclick prop', () => { | ||
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.
|
||
it('is not by default', () => { | ||
shallow(<Breadcrumb.Section>Home</Breadcrumb.Section>) | ||
.should.have.tagName('div') | ||
}) | ||
|
||
it('is should run click by prop', () => { | ||
const handleClick = sandbox.spy() | ||
const wrapper = shallow(<Breadcrumb.Section onClick={handleClick}>Home</Breadcrumb.Section>) | ||
|
||
wrapper.should.have.tagName('a') | ||
wrapper.should.have.attr('href').and.equal('#') | ||
|
||
wrapper.simulate('click') | ||
handleClick.should.have.been.calledOnce() | ||
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. Since the test states that the spy is called with an (event), we should also assert the first argument matches an object: handleClick.should.have.been.calledOnce()
handleClick.should.have.been.calledWithMatch({}) |
||
}) | ||
|
||
it('is shouldn not run click by prop, if prop not function', () => { | ||
const handleClick = sandbox.spy() | ||
const wrapper = shallow(<Breadcrumb.Section onClick='test'>Home</Breadcrumb.Section>) | ||
|
||
wrapper.should.have.tagName('a') | ||
wrapper.should.have.attr('href').and.equal('#') | ||
|
||
wrapper.simulate('click') | ||
handleClick.should.have.callCount(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.
Regarding the
render()
function:renderSections()
.key
, see dynamic childrenchildren
here is a 2 dimensional array with a the last nested child beingnull
, this could be a flat array with no null values.{ ...section }
is also going to spread props we don't need, liketext
. suggest destructuringtext
and spreading the...rest
.Some refactoring to address the above may look something like this: