Skip to content
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

Merged
merged 29 commits into from
Jul 17, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f28ba81
Breadcrumb component
Jul 2, 2016
cc969f1
Breadcrumb.Section #321
Jul 5, 2016
a87bb2e
Breadcrumb.Section #321
Jul 5, 2016
6eff245
Merge branch 'master' of /~https://github.com/TechnologyAdvice/stardust…
layershifter Jul 5, 2016
3893a03
README.md update #321
layershifter Jul 5, 2016
c01c4a6
(refactor) Breadcrumb.Section #321
layershifter Jul 5, 2016
23b3df4
(fix) Breadcrumb.Section test #321
layershifter Jul 5, 2016
80a2803
(fix) Breadcrumb.Section #321
layershifter Jul 5, 2016
f3864ae
(fix) Breadcrumb.Section #321
Jul 6, 2016
c2cbba0
(feat) Breadcrumb.Section #321
Jul 6, 2016
ffc09a5
(fix) Breadcrumb.Divider #321
Jul 6, 2016
43deaf1
Merge branch 'master' of /~https://github.com/TechnologyAdvice/stardust…
Jul 6, 2016
3d88988
(feat) Breadcrumb #321
Jul 6, 2016
c91d0d9
(fix) Breadcrumb.Section test #321
layershifter Jul 6, 2016
6fa9e5a
(fix) Breadcrumb.Section test #321
layershifter Jul 6, 2016
6937755
(fix) Breadcrumb #321
layershifter Jul 6, 2016
77baebb
(feat) Breadcrumb #321
Jul 7, 2016
25eb1bb
Merge branch 'master' of /~https://github.com/TechnologyAdvice/stardust…
Jul 7, 2016
92c13d2
(fix) Breadcrumb key #321
layershifter Jul 7, 2016
a44fb59
(feat) Breadcrumb test #321
layershifter Jul 7, 2016
82b04fa
(feat) Breadcrumb tests #321
Jul 8, 2016
a9ce2e3
(feat) Breadcrumb docs #321
Jul 8, 2016
a299332
(fix) Breadcrumb docs #321
Jul 11, 2016
32b53d4
(fix) Breadcrumb #321
Jul 11, 2016
a6f0654
Merge branch 'master' of /~https://github.com/TechnologyAdvice/stardust…
Jul 15, 2016
9edb4a0
(fix) Breadcrumb docs
Jul 15, 2016
ba14592
(fix) Breadcrumb
Jul 15, 2016
0f5235b
Merge branch 'master' of /~https://github.com/TechnologyAdvice/stardust…
layershifter Jul 17, 2016
9398fa5
(Breadcrumb) Test update
layershifter Jul 17, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions src/collections/Breadcrumb/Breadcrumb.js
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>
)
Copy link
Member

Choose a reason for hiding this comment

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

Regarding the render() function:

  • prefer to pull js expressions into variables or smaller render methods, i.e. renderSections().
  • children in an array need a unique key, see dynamic children
  • children here is a 2 dimensional array with a the last nested child being null, this could be a flat array with no null values.
  • spreading section { ...section } is also going to spread props we don't need, like text. suggest destructuring text and spreading the ...rest.

Some refactoring to address the above may look something like this:

const sectionsJSX = []

sections.forEach(({ text, ...restSection }, index) => {
  const key = `${text}-${index}`

  sectionsJSX.push(
    <Breadcrumb.Section {...restSection} key={key}>{text}</Breadcrumb.Section>
  )

  if (index !== sections.length - 1) {
    sectionsJSX.push(React.cloneElement(dividerJSX, { key }))
  }
})

return <div className={classes} {...rest}>{sectionsJSX}</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
95 changes: 95 additions & 0 deletions src/collections/Breadcrumb/BreadcrumbSection.js
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
Copy link
Member

Choose a reason for hiding this comment

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

The @see tag will show a link to a related component on the doc site:

image

Subcomponents however do not yet show up on the doc site. Let's remove the @see tag here since it will not do anything.


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.
* */
Copy link
Member

Choose a reason for hiding this comment

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

Reserve doc block comments (/**) for docs and public methods/members. Since this function cannot be used outside of this class, the doc block can be removed.

const handleClick = (e) => {
if (!_.isUndefined(e)) e.preventDefault()
if (!_.isFunction(onClick)) return

onClick()
}
Copy link
Member

Choose a reason for hiding this comment

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

Few notes on event handlers.

  1. Never preventDefault (the user can prevent default if they want)
  2. Always pass back the event
  3. propTypes validates the onClick as being a function for us, so we don't have to include validation

The refactored handleClick would look like this:

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 onClick prop on the returned component. When called, it will also be called with the event.

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',
Copy link
Member

Choose a reason for hiding this comment

The 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.
Copy link
Member

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

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

PropTypes are self documented as optional/required via the .isRequired property so the "optional" word is not needed.

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:

image

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>) */
Copy link
Member

Choose a reason for hiding this comment

The 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. */
Copy link
Member

Choose a reason for hiding this comment

The 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,
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you tell us more? If I remove onClick from propTypes, eslint fails with: error 'onClick' is missing in props validation react/prop-types

}

export default BreadcrumbSection
2 changes: 2 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ export Select from './addons/Select/Select'
// Collections
// ----------------------------------------

export Breadcrumb from './collections/Breadcrumb/Breadcrumb'

import _Form from './collections/Form/Form'
export { _Form as Form }
export const Field = deprecateComponent('Field', 'Use "Form.Field" instead.', _Form.Field)
Expand Down
15 changes: 15 additions & 0 deletions test/specs/collections/Breadcrumb/Breadcrumb-test.js
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')
})
})
84 changes: 84 additions & 0 deletions test/specs/collections/Breadcrumb/BreadcrumbSection-test.js
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'
Copy link
Member

Choose a reason for hiding this comment

The 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', () => {
Copy link
Member

Choose a reason for hiding this comment

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

For props that convert to classNames, we have propUtils for extracting the classes and commonTests for testing them. Have a look at CONTRIBUTING.md under the Prop Utils and the following section Testing className for how to use them. Let me know if you have any questions after that.

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', () => {
Copy link
Member

Choose a reason for hiding this comment

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

onClick

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()
Copy link
Member

Choose a reason for hiding this comment

The 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)
})
})
})