[changed] remove extra wrapping <nav> element in Nav components

react-bootstrap · Nov 14, 2015 · 59c9571 · 59c9571
1 parent fac0095
commit 59c9571
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}>