Skip to content

Commit

Permalink
Refactor duplication-removing logic for CollapsibleNav.
Browse files Browse the repository at this point in the history
Because of `-2` this code `indexOf('navbar-collapse') === -2` hasn't been working anyway.
But tests were green because the resulted logic was right someway.

This commit refactors it to clarify deduplication logic
by using `classnames'` [Alternate `dedupe` version](JedWatson/classnames@9acbd433ee)

`classes['navbar-collapse'] = collapsible`
=>
`let classes = collapsible ? this.getCollapsibleClassSet('navbar-collapse')`

`getDOMNode()` => `React.findDOMNode()`

And test was refactored too.
We can count class occurrences much simpler as
```js
let occurrences = (classDOM.match(/navbar-collapse/g) || []).length
```
  • Loading branch information
AlexKVal committed May 25, 2015
1 parent ed469cf commit e021050
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 35 deletions.
14 changes: 2 additions & 12 deletions src/CollapsibleNav.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,10 @@ const specCollapsibleNav = {

render() {
/*
* this.props.collapsible is set in NavBar when a eventKey is supplied.
* this.props.collapsible is set in NavBar when an eventKey is supplied.
*/
const collapsible = this.props.collapsible || this.props.collapsable;
let classes = collapsible ? this.getCollapsibleClassSet() : {};
/*
* prevent duplicating navbar-collapse call if passed as prop.
* kind of overkill...
* good cadidate to have check implemented as an util that can
* also be used elsewhere.
*/
if (this.props.className === undefined ||
this.props.className.split(' ').indexOf('navbar-collapse') === -2) {
classes['navbar-collapse'] = collapsible;
}
let classes = collapsible ? this.getCollapsibleClassSet('navbar-collapse') : null;

return (
<div eventKey={this.props.eventKey} className={classNames(this.props.className, classes)} >
Expand Down
24 changes: 1 addition & 23 deletions test/CollapsibleNavSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('CollapsibleNav', function () {
});
let instance = ReactTestUtils.renderIntoDocument(<Parent />);
let collapsibleNav = ReactTestUtils.findRenderedComponentWithType(instance, CollapsibleNav);
assert.notOk(collapsibleNav.getDOMNode().className.match(/\navbar-collapse\b/));
assert.notOk(React.findDOMNode(collapsibleNav).className.match(/\navbar-collapse\b/));
let nav = ReactTestUtils.findRenderedComponentWithType(collapsibleNav.refs.nocollapse_0, Nav);
assert.ok(nav);
});
Expand All @@ -89,26 +89,4 @@ describe('CollapsibleNav', function () {
assert.ok(ReactTestUtils.findRenderedDOMComponentWithClass(instance.refs.collapsible_object.refs.collapsible_0, 'bar'));
assert.ok(ReactTestUtils.findRenderedDOMComponentWithClass(instance.refs.collapsible_object.refs.collapsible_0, 'baz'));
});

it('Should should not duplicate classes', function () {
let Parent = React.createClass({
render() {
return (
<Navbar toggleNavKey={1}>
<CollapsibleNav eventKey={1} ref='collapsible_object' className='foo navbar-collapse'>
<Nav>
<NavItem eventKey={1} ref='item1' className='foo bar'>Item 1 content</NavItem>
<NavItem eventKey={2} ref='item2' className='baz'>Item 2 content</NavItem>
</Nav>
</CollapsibleNav>
</Navbar>
);
}
});
let instance = ReactTestUtils.renderIntoDocument(<Parent />);
let classDOM = ReactTestUtils.findRenderedDOMComponentWithTag(instance.refs.collapsible_object, 'DIV').props.className
, classArray = classDOM.split(' ')
, idx = classArray.indexOf('navbar-collapse');
assert.equal(classArray.indexOf('navbar-collapse', idx + 1), -1);
});
});

0 comments on commit e021050

Please sign in to comment.