Merge pull request react-bootstrap#1494 from react-bootstrap/no-nav-wrap

[changed] remove extra wrapping `<nav>` element in Nav components
Leonard-Li777 · Nov 15, 2015 · 60a7b07 · 60a7b07
2 parents f6b501e + 59c9571
commit 60a7b07
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 109 deletions.
diff --git a/src/Collapse.js b/src/Collapse.js
@@ -1,5 +1,6 @@
 import css from 'dom-helpers/style';
 import React from 'react';
+import classNames from 'classnames';
 import Transition from 'react-overlays/lib/Transition';
 import deprecated from 'react-prop-types/lib/deprecated';
 
@@ -50,7 +51,7 @@ class Collapse extends React.Component {
         ref="transition"
         {...this.props}
         aria-expanded={this.props.role ? this.props.in : null}
-        className={this._dimension() === 'width' ? 'width' : ''}
+        className={classNames(this.props.className, { width: this._dimension() === 'width' })}
         exitedClassName="collapse"
         exitingClassName="collapsing"
         enteredClassName="collapse in"

diff --git a/src/Fade.js b/src/Fade.js
@@ -1,4 +1,5 @@
 import React from 'react';
+import classNames from 'classnames';
 import Transition from 'react-overlays/lib/Transition';
 import deprecated from 'react-prop-types/lib/deprecated';
 
@@ -10,7 +11,7 @@ class Fade extends React.Component {
       <Transition
         {...this.props}
         timeout={timeout}
-        className="fade"
+        className={classNames(this.props.className, 'fade')}
         enteredClassName="in"
         enteringClassName="in"
       >

diff --git a/src/Nav.js b/src/Nav.js
@@ -2,48 +2,53 @@ import React, { cloneElement } from 'react';
 import classNames from 'classnames';
 import Collapse from './Collapse';
 import all from 'react-prop-types/lib/all';
+import deprecated from 'react-prop-types/lib/deprecated';
 import tbsUtils, { bsStyles, bsClass } from './utils/bootstrapUtils';
 import ValidComponentChildren from './utils/ValidComponentChildren';
 import createChainedFunction from './utils/createChainedFunction';
 
 class Nav extends React.Component {
 
   render() {
-    const classes = this.props.collapsible ? 'navbar-collapse' : null;
-
-    if (this.props.navbar && !this.props.collapsible) {
-      return (this.renderUl());
-    }
-
-    return (
-      <Collapse in={this.props.expanded}>
-        <nav {...this.props} className={classNames(this.props.className, classes)}>
-          {this.renderUl()}
-        </nav>
-      </Collapse>
-    );
-  }
-
-  renderUl() {
+    const { className, ulClassName, id, ulId } = this.props;
     const classes = tbsUtils.getClassSet(this.props);
 
-    // TODO: need to pass navbar bsClass down...
     classes[tbsUtils.prefix(this.props, 'stacked')] = this.props.stacked;
     classes[tbsUtils.prefix(this.props, 'justified')] = this.props.justified;
-    classes['navbar-nav'] = this.props.navbar;
-    classes['pull-right'] = this.props.pullRight;
-    classes['navbar-right'] = this.props.right;
 
-    return (
-      <ul {...this.props}
+    if (this.props.navbar) {
+      // TODO: need to pass navbar bsClass down...
+      classes['navbar-nav'] = this.props.navbar;
+      classes['navbar-right'] = this.props.right; // this is confusing along with `pullRight`
+    } else {
+      classes['pull-right'] = this.props.pullRight;
+    }
+
+    let list = (
+      <ul ref="ul"
+        {...this.props}
+        id={ulId || id}
         role={this.props.bsStyle === 'tabs' ? 'tablist' : null}
-        className={classNames(this.props.ulClassName, classes)}
-        id={this.props.ulId}
-        ref="ul"
+        className={classNames(className, ulClassName, classes)}
       >
         {ValidComponentChildren.map(this.props.children, this.renderNavItem, this)}
       </ul>
     );
+
+    if (this.props.collapsible) {
+      list = (
+        <Collapse
+          in={this.props.expanded}
+          className={this.props.navbar ? 'navbar-collapse' : void 0}
+        >
+          <div>
+            { list }
+          </div>
+        </Collapse>
+      );
+    }
+
+    return list;
   }
 
   getChildActiveProp(child) {
@@ -107,12 +112,19 @@ Nav.propTypes = {
   ]),
   /**
    * CSS classes for the inner `ul` element
+   *
+   * @deprecated
    */
-  ulClassName: React.PropTypes.string,
+  ulClassName: deprecated(React.PropTypes.string,
+    'The wrapping `<nav>` has been removed you can use `className` now'),
   /**
    * HTML id for the inner `ul` element
+   *
+   * @deprecated
    */
-  ulId: React.PropTypes.string,
+  ulId: deprecated(React.PropTypes.string,
+    'The wrapping `<nav>` has been removed you can use `id` now'),
+
   expanded: React.PropTypes.bool,
   navbar: React.PropTypes.bool,
   eventKey: React.PropTypes.any,

diff --git a/test/NavSpec.js b/test/NavSpec.js
@@ -65,7 +65,7 @@ describe('Nav', () => {
 
   it('Should add navbar-right class', () => {
     let instance = ReactTestUtils.renderIntoDocument(
-          <Nav bsStyle="tabs" right activeKey={1}>
+          <Nav bsStyle="tabs" navbar right activeKey={1}>
             <NavItem key={1}>Tab 1 content</NavItem>
             <NavItem key={2}>Tab 2 content</NavItem>
           </Nav>
@@ -118,68 +118,6 @@ describe('Nav', () => {
     assert.ok(items[0].props.navItem);
   });
 
-  it('Should apply className only to the wrapper nav element', () => {
-    const instance = ReactTestUtils.renderIntoDocument(
-      <Nav bsStyle="tabs" activeKey={1} className="nav-specific">
-        <NavItem key={1}>Tab 1 content</NavItem>
-        <NavItem key={2}>Tab 2 content</NavItem>
-      </Nav>
-    );
-
-    let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
-    assert.notInclude(ulNode.className, 'nav-specific');
-
-    let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
-    assert.include(navNode.className, 'nav-specific');
-  });
-
-  it('Should apply ulClassName to the inner ul element', () => {
-    const instance = ReactTestUtils.renderIntoDocument(
-      <Nav bsStyle="tabs" activeKey={1} className="nav-specific" ulClassName="ul-specific">
-        <NavItem key={1}>Tab 1 content</NavItem>
-        <NavItem key={2}>Tab 2 content</NavItem>
-      </Nav>
-    );
-
-    let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
-    assert.include(ulNode.className, 'ul-specific');
-    assert.notInclude(ulNode.className, 'nav-specific');
-
-    let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
-    assert.notInclude(navNode.className, 'ul-specific');
-    assert.include(navNode.className, 'nav-specific');
-  });
-
-  it('Should apply id to the wrapper nav element', () => {
-    const instance = ReactTestUtils.renderIntoDocument(
-      <Nav bsStyle="tabs" activeKey={1} id="nav-id">
-        <NavItem key={1}>Tab 1 content</NavItem>
-        <NavItem key={2}>Tab 2 content</NavItem>
-      </Nav>
-    );
-
-    let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
-    assert.equal(navNode.id, 'nav-id');
-
-    let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
-    assert.notEqual(ulNode.id, 'nav-id');
-  });
-
-  it('Should apply ulId to the inner ul element', () => {
-    const instance = ReactTestUtils.renderIntoDocument(
-      <Nav bsStyle="tabs" activeKey={1} id="nav-id" ulId="ul-id">
-        <NavItem key={1}>Tab 1 content</NavItem>
-        <NavItem key={2}>Tab 2 content</NavItem>
-      </Nav>
-    );
-
-    let ulNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'ul');
-    assert.equal(ulNode.id, 'ul-id');
-
-    let navNode = ReactTestUtils.findRenderedDOMComponentWithTag(instance, 'nav');
-    assert.equal(navNode.id, 'nav-id');
-  });
-
   it('Should warn when attempting to use a justified navbar nav', () => {
     ReactTestUtils.renderIntoDocument(
       <Nav navbar justified />

diff --git a/test/NavbarSpec.js b/test/NavbarSpec.js
@@ -6,7 +6,7 @@ import Nav from '../src/Nav';
 import NavBrand from '../src/NavBrand';
 import Navbar from '../src/Navbar';
 
-import {getOne, render, shouldWarn} from './helpers';
+import { getOne, shouldWarn } from './helpers';
 
 describe('Navbar', () => {
 
@@ -168,22 +168,6 @@ describe('Navbar', () => {
     assert.ok(nav.props.navbar);
   });
 
-  it('Should pass nav prop to ul', () => {
-    let instance = render(<Nav />);
-
-    let navNode = ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'nav');
-    assert.ok(navNode);
-    assert.equal(navNode.nodeName, 'UL');
-    assert.equal(navNode.parentNode.nodeName, 'NAV');
-
-    instance = instance.renderWithProps({navbar: true});
-
-    navNode = ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'nav');
-    assert.ok(navNode);
-    assert.equal(navNode.nodeName, 'UL');
-    assert.equal(navNode.parentNode.nodeName, 'DIV');
-  });
-
   it('Should add header when toggleNavKey is 0', () => {
     let instance = ReactTestUtils.renderIntoDocument(
       <Navbar toggleNavKey={0}>