Wc 24/gridlist components

[akdetrick]

Related issues

Fixes https://meetup.atlassian.net/browse/WC-24

Description

Add GridList component

Screenshots (if applicable)

[coveralls]

Coverage increased (+0.4%) to 84.17% when pulling e29e872 on WC-24/gridlist_components into fc65369 on master.

[eilinora]

Instead of findDOMNode we've been trying to use these the TestUtils functions like https://facebook.github.io/react/docs/test-utils.html#findRenderedDOMComponentWithClass for these kinds of tests, where we can. There are definitely still instances where you can't get away from avoiding findDOMNode, but since we are testing the classes components directly applied to the component, this should work. You can do something like

const gridList = TestUtils.findRenderedDOMComponentWithClass(gridList, GridList);
const gridListClassList = gridList.props.className;

[akdetrick]

What are the advantages of using this TestUtils method over ReactDOM?

There are some convenience methods in ReactDOM that were nice, like node.classList.contains('someClass'), as opposed to component.props.className.indexOf('someClass') > -1

[mmcgahan]

@eilinora shared a nice article in another PR related to this: http://reactkungfu.com/2015/07/approaches-to-testing-react-components-an-overview/

One of the principles is that you should 'maintain the component abstraction in your tests as long as possible, but no longer', i.e. you should definitely prefer TestUtils methods that scan the React tree, but if you really need to get details of the DOM, you can drop into the 'real' DOM for some tests. I think one of the challenges of working with React is breaking the mental connection between 'React component tree' and 'DOM tree', and then determining at each step of application development which of those abstractions you are desiging/testing.

The article is good, though - definitely worth your time.

[akdetrick]

I was mistaken; Jest makes this easy...

expect(someClassList).toContain(aClass);

fixed

[coveralls]

Coverage increased (+0.3%) to 86.242% when pulling c08cfaa on WC-24/gridlist_components into 2be2da6 on master.

[akdetrick]

bump

Good to merge?

[mmcgahan]

prefer to keep your 'type-enforcement' code in one place - since number is already 'enforced' by the proptypes, I recommend just checking for the existence of the prop rather than the type here

[`gridList--has${columns.default}`]: columns.default  // or Boolean(columns.default)

[mmcgahan]

given how small the render method is for this component, I think it would be more direct to just put this renderItems method inside render directly rather than requiring engineers to jump up to this method when debugging a render error. The whole JSX block in render could be tightened up without obscuring things too much:

render() {
  // ...
  return (
    <ul className={classNames} {...other}>
      {items.map((item, key) =>
          <div key={key} className='gridList-item'>{item}</div>
      )}
    </ul>
  );
}

fixed

[coveralls]

Coverage increased (+0.2%) to 86.149% when pulling ac449a7 on WC-24/gridlist_components into 1a5ffe7 on master.

[eilinora]

gonna approve but this would be nice :)

[eilinora]

super obnoxious nitpick on my part can you do > on new line makes it easier to read

[coveralls]

Coverage increased (+0.5%) to 86.379% when pulling 1acaba3 on WC-24/gridlist_components into 1a5ffe7 on master.