Skip to content

Commit 59c9571

Browse files
jquensejquense
committed
[changed] remove extra wrapping <nav> element in Nav components
1 parent fac0095 commit 59c9571

File tree

5 files changed

+45
-109
lines changed

5 files changed

+45
-109
lines changed

‎src/Collapse.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import css from 'dom-helpers/style';
22
import React from 'react';
3+
import classNames from 'classnames';
34
import Transition from 'react-overlays/lib/Transition';
45
import deprecated from 'react-prop-types/lib/deprecated';
56

@@ -50,7 +51,7 @@ class Collapse extends React.Component {
5051
ref="transition"
5152
{...this.props}
5253
aria-expanded={this.props.role ? this.props.in : null}
53-
className={this._dimension() === 'width' ? 'width' : ''}
54+
className={classNames(this.props.className, { width: this._dimension() === 'width' })}
5455
exitedClassName="collapse"
5556
exitingClassName="collapsing"
5657
enteredClassName="collapse in"

‎src/Fade.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import React from 'react';
2+
import classNames from 'classnames';
23
import Transition from 'react-overlays/lib/Transition';
34
import deprecated from 'react-prop-types/lib/deprecated';
45

@@ -10,7 +11,7 @@ class Fade extends React.Component {
1011
<Transition
1112
{...this.props}
1213
timeout={timeout}
13-
className="fade"
14+
className={classNames(this.props.className, 'fade')}
1415
enteredClassName="in"
1516
enteringClassName="in"
1617
>

‎src/Nav.js

Lines changed: 39 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,48 +2,53 @@ import React, { cloneElement } from 'react';
22
import classNames from 'classnames';
33
import Collapse from './Collapse';
44
import all from 'react-prop-types/lib/all';
5+
import deprecated from 'react-prop-types/lib/deprecated';
56
import tbsUtils, { bsStyles, bsClass } from './utils/bootstrapUtils';
67
import ValidComponentChildren from './utils/ValidComponentChildren';
78
import createChainedFunction from './utils/createChainedFunction';
89

910
class Nav extends React.Component {
1011

1112
render() {
12-
const classes = this.props.collapsible ? 'navbar-collapse' : null;
13-
14-
if (this.props.navbar && !this.props.collapsible) {
15-
return (this.renderUl());
16-
}
17-
18-
return (
19-
<Collapse in={this.props.expanded}>
20-
<nav {...this.props} className={classNames(this.props.className, classes)}>
21-
{this.renderUl()}
22-
</nav>
23-
</Collapse>
24-
);
25-
}
26-
27-
renderUl() {
13+
const { className, ulClassName, id, ulId } = this.props;
2814
const classes = tbsUtils.getClassSet(this.props);
2915

30-
// TODO: need to pass navbar bsClass down...
3116
classes[tbsUtils.prefix(this.props, 'stacked')] = this.props.stacked;
3217
classes[tbsUtils.prefix(this.props, 'justified')] = this.props.justified;
33-
classes['navbar-nav'] = this.props.navbar;
34-
classes['pull-right'] = this.props.pullRight;
35-
classes['navbar-right'] = this.props.right;
3618

37-
return (
38-
<ul {...this.props}
19+
if (this.props.navbar) {
20+
// TODO: need to pass navbar bsClass down...
21+
classes['navbar-nav'] = this.props.navbar;
22+
classes['navbar-right'] = this.props.right; // this is confusing along with `pullRight`
23+
} else {
24+
classes['pull-right'] = this.props.pullRight;
25+
}
26+
27+
let list = (
28+
<ul ref="ul"
29+
{...this.props}
30+
id={ulId || id}
3931
role={this.props.bsStyle === 'tabs' ? 'tablist' : null}
40-
className={classNames(this.props.ulClassName, classes)}
41-
id={this.props.ulId}
42-
ref="ul"
32+
className={classNames(className, ulClassName, classes)}
4333
>
4434
{ValidComponentChildren.map(this.props.children, this.renderNavItem, this)}
4535
</ul>
4636
);
37+
38+
if (this.props.collapsible) {
39+
list = (
40+
<Collapse
41+
in={this.props.expanded}
42+
className={this.props.navbar ? 'navbar-collapse' : void 0}
43+
>
44+
<div>
45+
{ list }
46+
</div>
47+
</Collapse>
48+
);
49+
}
50+
51+
return list;
4752
}
4853

4954
getChildActiveProp(child) {
@@ -107,12 +112,19 @@ Nav.propTypes = {
107112
]),
108113
/**
109114
* CSS classes for the inner `ul` element
115+
*
116+
* @deprecated
110117
*/
111-
ulClassName: React.PropTypes.string,
118+
ulClassName: deprecated(React.PropTypes.string,
119+
'The wrapping `<nav>` has been removed you can use `className` now'),
112120
/**
113121
* HTML id for the inner `ul` element
122+
*
123+
* @deprecated
114124
*/
115-
ulId: React.PropTypes.string,
125+
ulId: deprecated(React.PropTypes.string,
126+
'The wrapping `<nav>` has been removed you can use `id` now'),
127+
116128
expanded: React.PropTypes.bool,
117129
navbar: React.PropTypes.bool,
118130
eventKey: React.PropTypes.any,

‎test/NavSpec.js

Lines changed: 1 addition & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ describe('Nav', () => {
6565

6666
it('Should add navbar-right class', () => {
6767
let instance = ReactTestUtils.renderIntoDocument(
68-
<Nav bsStyle="tabs" right activeKey={1}>
68+
<Nav bsStyle="tabs" navbar right activeKey={1}>
6969
<NavItem key={1}>Tab 1 content</NavItem>
7070
<NavItem key={2}>Tab 2 content</NavItem>
7171
</Nav>
@@ -118,68 +118,6 @@ describe('Nav', () => {
118118
assert.ok(items[0].props.navItem);
119119
});
120120

121-
it('Should apply className only to the wrapper nav element', () => {
122-
const instance = ReactTestUtils.renderIntoDocument(
123-
<Nav bsStyle="tabs" activeKey={1} className="nav-specific">
124-
<NavItem key={1}>Tab 1 content</NavItem>
125-
<NavItem key={2}>Tab 2 content</NavItem>
126-
</Nav>
127-
);
128-
129-
let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
130-
assert.notInclude(ulNode.className, 'nav-specific');
131-
132-
let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
133-
assert.include(navNode.className, 'nav-specific');
134-
});
135-
136-
it('Should apply ulClassName to the inner ul element', () => {
137-
const instance = ReactTestUtils.renderIntoDocument(
138-
<Nav bsStyle="tabs" activeKey={1} className="nav-specific" ulClassName="ul-specific">
139-
<NavItem key={1}>Tab 1 content</NavItem>
140-
<NavItem key={2}>Tab 2 content</NavItem>
141-
</Nav>
142-
);
143-
144-
let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
145-
assert.include(ulNode.className, 'ul-specific');
146-
assert.notInclude(ulNode.className, 'nav-specific');
147-
148-
let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
149-
assert.notInclude(navNode.className, 'ul-specific');
150-
assert.include(navNode.className, 'nav-specific');
151-
});
152-
153-
it('Should apply id to the wrapper nav element', () => {
154-
const instance = ReactTestUtils.renderIntoDocument(
155-
<Nav bsStyle="tabs" activeKey={1} id="nav-id">
156-
<NavItem key={1}>Tab 1 content</NavItem>
157-
<NavItem key={2}>Tab 2 content</NavItem>
158-
</Nav>
159-
);
160-
161-
let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
162-
assert.equal(navNode.id, 'nav-id');
163-
164-
let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
165-
assert.notEqual(ulNode.id, 'nav-id');
166-
});
167-
168-
it('Should apply ulId to the inner ul element', () => {
169-
const instance = ReactTestUtils.renderIntoDocument(
170-
<Nav bsStyle="tabs" activeKey={1} id="nav-id" ulId="ul-id">
171-
<NavItem key={1}>Tab 1 content</NavItem>
172-
<NavItem key={2}>Tab 2 content</NavItem>
173-
</Nav>
174-
);
175-
176-
let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
177-
assert.equal(ulNode.id, 'ul-id');
178-
179-
let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
180-
assert.equal(navNode.id, 'nav-id');
181-
});
182-
183121
it('Should warn when attempting to use a justified navbar nav', () => {
184122
ReactTestUtils.renderIntoDocument(
185123
<Nav navbar justified />

‎test/NavbarSpec.js

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import Nav from '../src/Nav';
66
import NavBrand from '../src/NavBrand';
77
import Navbar from '../src/Navbar';
88

9-
import {getOne, render, shouldWarn} from './helpers';
9+
import { getOne, shouldWarn } from './helpers';
1010

1111
describe('Navbar', () => {
1212

@@ -168,22 +168,6 @@ describe('Navbar', () => {
168168
assert.ok(nav.props.navbar);
169169
});
170170

171-
it('Should pass nav prop to ul', () => {
172-
let instance = render(<Nav />);
173-
174-
let navNode = ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'nav');
175-
assert.ok(navNode);
176-
assert.equal(navNode.nodeName, 'UL');
177-
assert.equal(navNode.parentNode.nodeName, 'NAV');
178-
179-
instance = instance.renderWithProps({navbar: true});
180-
181-
navNode = ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'nav');
182-
assert.ok(navNode);
183-
assert.equal(navNode.nodeName, 'UL');
184-
assert.equal(navNode.parentNode.nodeName, 'DIV');
185-
});
186-
187171
it('Should add header when toggleNavKey is 0', () => {
188172
let instance = ReactTestUtils.renderIntoDocument(
189173
<Navbar toggleNavKey={0}>

0 commit comments

Comments
 (0)