From 501f0c52fe66c86d580aae22642c62916869647e Mon Sep 17 00:00:00 2001 From: Michael Ziwisky Date: Fri, 12 Feb 2016 01:02:15 -0600 Subject: [PATCH 1/2] enable controlled component behavior if you pass a `selectedIndex` prop, then that tab will be selected, no matter what. it doesn't change automatically from user interaction. to change in response to interaction, have the parent pass an `onSelect` handler that updates the `selectedIndex` prop. --- README.md | 24 ++-- examples/controlled/app.js | 51 +++++++++ examples/controlled/index.html | 8 ++ lib/components/Tab.js | 6 +- lib/components/Tabs.js | 144 +++++++----------------- lib/components/__tests__/Tabs-test.js | 151 +++++++++++++++++--------- 6 files changed, 216 insertions(+), 168 deletions(-) create mode 100644 examples/controlled/app.js create mode 100644 examples/controlled/index.html diff --git a/README.md b/README.md index c85afe64ee..d210b4d0ff 100644 --- a/README.md +++ b/README.md @@ -24,8 +24,12 @@ var TabList = ReactTabs.TabList; var TabPanel = ReactTabs.TabPanel; var App = React.createClass({ - handleSelect: function (index, last) { - console.log('Selected tab: ' + index + ', Last tab: ' + last); + getInitialState: function () { + return { selectedTab: 2 }; + }, + + handleSelect: function (index) { + this.setState({ selectedTab: index }); }, render: function () { @@ -33,11 +37,17 @@ var App = React.createClass({ {/* is a composite component and acts as the main container. - `onSelect` is called whenever a tab is selected. The handler for - this function will be passed the current index as well as the last index. + `selectedIndex` is the currently selected tab. + + `onSelect` is a callback invoked whenever a user clicks on or + keyboard-navigates to a tab. It is passed the index of the selected tab. - `selectedIndex` is the tab to select when first rendered. By default - the first (index 0) tab will be selected. + If you provide a `selectedIndex`, you should always provide an `onSelect` + (and vice versa) so that you can update the `selectedIndex` in response to + user interactions. If you pass neither of these props, then the component's + default "uncontrolled" behavior is to automatically update its selectedIndex + to whatever would be passed to the onSelect handler. I.e., it behaves as + you'd expect. `forceRenderTabPanel` By default this react-tabs will only render the selected tab's contents. Setting `forceRenderTabPanel` to `true` allows you to override the @@ -47,7 +57,7 @@ var App = React.createClass({ {/* diff --git a/examples/controlled/app.js b/examples/controlled/app.js new file mode 100644 index 0000000000..fa08993346 --- /dev/null +++ b/examples/controlled/app.js @@ -0,0 +1,51 @@ +import React from 'react'; +import ReactDOM from 'react-dom'; +import { Tab, Tabs, TabList, TabPanel } from '../../lib/main'; + +const App = React.createClass({ + handleInputChange() { + this.forceUpdate(); + }, + + getInitialState() { + return {currentTab: 1}; + }, + + handleTabSelect(index) { + this.setState({currentTab: index}); + }, + + render() { + return ( +
+ + + + First + Second + + +

This is the first tab, but not the default.

+ + +

This is the default tab.

+ + + +
+ ); + }, + + swapTab() { + this.setState({currentTab: (this.state.currentTab + 1) % 2}); + } +}); + +ReactDOM.render(, document.getElementById('example')); diff --git a/examples/controlled/index.html b/examples/controlled/index.html new file mode 100644 index 0000000000..1507df3939 --- /dev/null +++ b/examples/controlled/index.html @@ -0,0 +1,8 @@ + + +React Tabs + +
+ + + diff --git a/lib/components/Tab.js b/lib/components/Tab.js index 51090143ed..f8e5478b1c 100644 --- a/lib/components/Tab.js +++ b/lib/components/Tab.js @@ -3,12 +3,11 @@ import { findDOMNode } from 'react-dom'; import cx from 'classnames'; function syncNodeAttributes(node, props) { + // tabindex and selected misbehave when trying to set them like other JSX + // attributes. see https://github.com/facebook/react/issues/2528 if (props.selected) { node.setAttribute('tabindex', 0); node.setAttribute('selected', 'selected'); - if (props.focus) { - node.focus(); - } } else { node.removeAttribute('tabindex'); node.removeAttribute('selected'); @@ -33,7 +32,6 @@ module.exports = React.createClass({ getDefaultProps() { return { - focus: false, selected: false, id: null, panelId: null diff --git a/lib/components/Tabs.js b/lib/components/Tabs.js index ad99d53fbe..3abd4b882c 100644 --- a/lib/components/Tabs.js +++ b/lib/components/Tabs.js @@ -24,7 +24,6 @@ module.exports = React.createClass({ className: PropTypes.string, selectedIndex: PropTypes.number, onSelect: PropTypes.func, - focus: PropTypes.bool, children: childrenPropType, forceRenderTabPanel: PropTypes.bool }, @@ -41,14 +40,14 @@ module.exports = React.createClass({ getDefaultProps() { return { - selectedIndex: -1, - focus: false, forceRenderTabPanel: false }; }, getInitialState() { - return this.copyPropsToState(this.props); + return this.selectedIndexPropProvided() ? {} : { + selectedIndex: 0 + }; }, getChildContext() { @@ -63,8 +62,11 @@ module.exports = React.createClass({ } }, - componentWillReceiveProps(newProps) { - this.setState(this.copyPropsToState(newProps)); + componentDidUpdate() { + if (this.needsFocus) { + this.needsFocus = false; + this.focusSelectedTab(); + } }, handleClick(e) { @@ -84,7 +86,7 @@ module.exports = React.createClass({ handleKeyDown(e) { if (isTabNode(e.target)) { - let index = this.state.selectedIndex; + let index = this.getSelectedIndex(); let preventDefault = false; // Select next tab to the left @@ -110,67 +112,42 @@ module.exports = React.createClass({ setSelected(index, focus) { // Don't do anything if nothing has changed - if (index === this.state.selectedIndex) return; - // Check index boundary - if (index < 0 || index >= this.getTabsCount()) return; - - // Keep reference to last index for event handler - const last = this.state.selectedIndex; + if (index === this.getSelectedIndex()) return; - // Update selected index - this.setState({ selectedIndex: index, focus: focus === true }); + // mark whether focus is needed after next re-render + this.needsFocus = focus; // Call change event handler if (typeof this.props.onSelect === 'function') { - this.props.onSelect(index, last); + this.props.onSelect(index); + } else { // default "uncontrolled component" behavior + this.setState({ selectedIndex: index }); } }, - getNextTab(index) { - const count = this.getTabsCount(); - - // Look for non-disabled tab from index to the last tab on the right - for (let i = index + 1; i < count; i++) { - const tab = this.getTab(i); - if (!isTabDisabled(findDOMNode(tab))) { - return i; - } - } + getSelectedIndex() { + const authority = this.selectedIndexPropProvided() ? this.props : this.state; + return authority.selectedIndex; + }, - // If no tab found, continue searching from first on left to index - for (let i = 0; i < index; i++) { - const tab = this.getTab(i); - if (!isTabDisabled(findDOMNode(tab))) { - return i; + // from `index`, step thru tabs looking for a non-disabled one, and + // return the index of the first one you find. if `increment` is 1, + // step towards the right. if it's -1, step towards the left. + getNextTab(index, increment = 1) { + const count = this.getTabsCount(); + let nextIndex; + let delta = count + increment; + for (let i = 0; i < count; i++, delta += increment) { + nextIndex = (index + delta) % count; + if (!isTabDisabled(findDOMNode(this.getTab(nextIndex)))) { + break; } } - - // No tabs are disabled, return index - return index; + return nextIndex; }, getPrevTab(index) { - let i = index; - - // Look for non-disabled tab from index to first tab on the left - while (i--) { - const tab = this.getTab(i); - if (!isTabDisabled(findDOMNode(tab))) { - return i; - } - } - - // If no tab found, continue searching from last tab on right to index - i = this.getTabsCount(); - while (i-- > index) { - const tab = this.getTab(i); - if (!isTabDisabled(findDOMNode(tab))) { - return i; - } - } - - // No tabs are disabled, return index - return index; + return this.getNextTab(index, -1); }, getTabsCount() { @@ -199,7 +176,6 @@ module.exports = React.createClass({ let index = 0; let count = 0; const children = this.props.children; - const state = this.state; const tabIds = this.tabIds = this.tabIds || []; const panelIds = this.panelIds = this.panelIds || []; let diff = this.tabIds.length - this.getTabsCount(); @@ -237,8 +213,7 @@ module.exports = React.createClass({ const ref = 'tabs-' + index; const id = tabIds[index]; const panelId = panelIds[index]; - const selected = state.selectedIndex === index; - const focus = selected && state.focus; + const selected = this.getSelectedIndex() === index; index++; @@ -246,8 +221,7 @@ module.exports = React.createClass({ ref, id, panelId, - selected, - focus + selected }); }) }); @@ -260,7 +234,7 @@ module.exports = React.createClass({ const ref = 'panels-' + index; const id = panelIds[index]; const tabId = tabIds[index]; - const selected = state.selectedIndex === index; + const selected = this.getSelectedIndex() === index; index++; @@ -277,25 +251,6 @@ module.exports = React.createClass({ }, render() { - // This fixes an issue with focus management. - // - // Ultimately, when focus is true, and an input has focus, - // and any change on that input causes a state change/re-render, - // focus gets sent back to the active tab, and input loses focus. - // - // Since the focus state only needs to be remembered - // for the current render, we can reset it once the - // render has happened. - // - // Don't use setState, because we don't want to re-render. - // - // See https://github.com/rackt/react-tabs/pull/7 - if (this.state.focus) { - setTimeout(() => { - this.state.focus = false; - }, 0); - } - return (
+ // + // First + // Second + // + // 1st + // + // + // ); + // } + // }); + // + // let tabs = TestUtils.renderIntoDocument().refs.tabs; + // let input = tabs.getDOMNode().querySelector('input'); + // + // input.focus(); + // TestUtils.Simulate.keyDown(input, { + // keyCode: 'a'.charCodeAt() + // }); + // + // assertTabSelected(tabs, 1); + // }); }); - it('should update selectedIndex when tab child is clicked', function() { - const tabs = TestUtils.renderIntoDocument(createTabs()); - - TestUtils.Simulate.click(findDOMNode(tabs.getTab(2)).firstChild); - assertTabSelected(tabs, 2); + describe('as a controlled component', function() { + it('should call onSelect when a tab is clicked', function() { + const called = {index: null}; + const tabs = TestUtils.renderIntoDocument(createTabs({ + onSelect: function(index) { + called.index = index; + } + })); + + TestUtils.Simulate.click(findDOMNode(tabs.getTab(1))); + equal(called.index, 1); + }); + + it('should not call onSelect when a disabled tab is clicked', function() { + const called = {index: null}; + const tabs = TestUtils.renderIntoDocument(createTabs({ + onSelect: function(index) { + called.index = index; + } + })); + + TestUtils.Simulate.click(findDOMNode(tabs.getTab(3))); + equal(called.index, null); + }); + + it('should not automatically update selected tab when clicked', function() { + const tabs = TestUtils.renderIntoDocument(createTabs({selectedIndex: 0})); + TestUtils.Simulate.click(findDOMNode(tabs.getTab(1))); + + assertTabSelected(tabs, 0); + }); + + it('should update selected tab when selectedIndex changes', function() { + const container = document.createElement('div'); + const tabs = render(createTabs({selectedIndex: 1}), container); + assertTabSelected(tabs, 1); + + render(createTabs({selectedIndex: 0}), container); + assertTabSelected(tabs, 0); + + unmountComponentAtNode(container); + }); }); - - it('should not change selectedIndex when clicking a disabled tab', function() { - const tabs = TestUtils.renderIntoDocument(createTabs({selectedIndex: 0})); - - TestUtils.Simulate.click(findDOMNode(tabs.getTab(3))); - assertTabSelected(tabs, 0); - }); - - // TODO: Can't seem to make this fail when removing fix :`( - // See https://github.com/rackt/react-tabs/pull/7 - // it('should preserve selectedIndex when typing', function () { - // let App = React.createClass({ - // handleKeyDown: function () { this.forceUpdate(); }, - // render: function () { - // return ( - // - // - // First - // Second - // - // 1st - // - // - // ); - // } - // }); - // - // let tabs = TestUtils.renderIntoDocument().refs.tabs; - // let input = tabs.getDOMNode().querySelector('input'); - // - // input.focus(); - // TestUtils.Simulate.keyDown(input, { - // keyCode: 'a'.charCodeAt() - // }); - // - // assertTabSelected(tabs, 1); - // }); }); describe('performance', function() { From 61dfa85f0719e7c519bd93672dac6e7698ca388e Mon Sep 17 00:00:00 2001 From: Michael Ziwisky Date: Fri, 19 Feb 2016 12:02:11 -0600 Subject: [PATCH 2/2] more reliable focus management for controlled component previously, in order for focus management to work, we assumed the parent would rerender the tabs component when its onSelect handler was invoked. that was probably a pretty safe assumption, but we no longer rely on it. also made it more explicit (and enforced) that `selectedIndex` and `onSelect` come as a pair. and improved test coverage around this stuff. --- lib/components/Tabs.js | 27 ++++--- lib/components/__tests__/Tabs-test.js | 101 +++++++++++++++++++++----- lib/helpers/onSelectPropType.js | 20 +++++ lib/helpers/selectedIndexPropType.js | 18 +++++ 4 files changed, 136 insertions(+), 30 deletions(-) create mode 100644 lib/helpers/onSelectPropType.js create mode 100644 lib/helpers/selectedIndexPropType.js diff --git a/lib/components/Tabs.js b/lib/components/Tabs.js index 3abd4b882c..d98f41838b 100644 --- a/lib/components/Tabs.js +++ b/lib/components/Tabs.js @@ -4,6 +4,8 @@ import cx from 'classnames'; import jss from 'js-stylesheet'; import uuid from '../helpers/uuid'; import childrenPropType from '../helpers/childrenPropType'; +import onSelectPropType from '../helpers/onSelectPropType'; +import selectedIndexPropType from '../helpers/selectedIndexPropType'; // Determine if a node from event.target is a Tab element function isTabNode(node) { @@ -22,8 +24,8 @@ module.exports = React.createClass({ propTypes: { className: PropTypes.string, - selectedIndex: PropTypes.number, - onSelect: PropTypes.func, + selectedIndex: selectedIndexPropType, + onSelect: onSelectPropType, children: childrenPropType, forceRenderTabPanel: PropTypes.bool }, @@ -45,7 +47,10 @@ module.exports = React.createClass({ }, getInitialState() { - return this.selectedIndexPropProvided() ? {} : { + return this.isControlledComponent() ? { + needsFocus: false + } : { + needsFocus: false, selectedIndex: 0 }; }, @@ -63,8 +68,9 @@ module.exports = React.createClass({ }, componentDidUpdate() { - if (this.needsFocus) { - this.needsFocus = false; + if (this.state.needsFocus) { + // we don't want this to trigger a re-render, so we avoid setState here + this.state.needsFocus = false; this.focusSelectedTab(); } }, @@ -115,10 +121,10 @@ module.exports = React.createClass({ if (index === this.getSelectedIndex()) return; // mark whether focus is needed after next re-render - this.needsFocus = focus; + this.setState({ needsFocus: !!focus }); // Call change event handler - if (typeof this.props.onSelect === 'function') { + if (this.isControlledComponent()) { this.props.onSelect(index); } else { // default "uncontrolled component" behavior this.setState({ selectedIndex: index }); @@ -126,7 +132,7 @@ module.exports = React.createClass({ }, getSelectedIndex() { - const authority = this.selectedIndexPropProvided() ? this.props : this.state; + const authority = this.isControlledComponent() ? this.props : this.state; return authority.selectedIndex; }, @@ -266,8 +272,9 @@ module.exports = React.createClass({ ); }, - selectedIndexPropProvided() { - return (typeof this.props.selectedIndex === 'number'); + isControlledComponent() { + return (typeof this.props.selectedIndex === 'number' && + typeof this.props.onSelect === 'function'); }, focusSelectedTab() { diff --git a/lib/components/__tests__/Tabs-test.js b/lib/components/__tests__/Tabs-test.js index c96ddadddd..ff3d5ce381 100644 --- a/lib/components/__tests__/Tabs-test.js +++ b/lib/components/__tests__/Tabs-test.js @@ -37,28 +37,54 @@ function assertTabSelected(tabs, index) { /* eslint func-names:0 */ describe('react-tabs', function() { describe('props', function() { - it('should default to selectedIndex being 0', function() { - const tabs = TestUtils.renderIntoDocument(createTabs()); - - assertTabSelected(tabs, 0); - }); + describe('selectedIndex', function() { + describe('when onSelect is present', function() { + it('should honor the prop', function() { + const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 1, + onSelect: () => {} + })); + + assertTabSelected(tabs, 1); + }); + }); - it('should honor selectedIndex prop', function() { - const tabs = TestUtils.renderIntoDocument(createTabs({selectedIndex: 1})); + describe('when onSelect is omitted', function() { + it('should ignore the prop', function() { + const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 1 + })); - assertTabSelected(tabs, 1); + assertTabSelected(tabs, 0); + }); + }); }); - it('should call onSelect when selection changes', function() { - const called = {index: -1}; - const tabs = TestUtils.renderIntoDocument(createTabs({ - onSelect: function(index) { - called.index = index; - } - })); + describe('onSelect', function() { + describe('when selectedIndex is present', function() { + it('should invoke the callback when selection changes', function() { + const called = {index: null}; + const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 1, + onSelect: (index) => { called.index = index; } + })); + + tabs.setSelected(2); + equal(called.index, 2); + }); + }); + + describe('when selectedIndex is omitted', function() { + it('should ignore the callback when selection changes', function() { + const called = {index: null}; + const tabs = TestUtils.renderIntoDocument(createTabs({ + onSelect: (index) => { called.index = index; } + })); - tabs.setSelected(2); - equal(called.index, 2); + tabs.setSelected(2); + equal(called.index, null); + }); + }); }); it('should have a default className', function() { @@ -99,6 +125,12 @@ describe('react-tabs', function() { describe('interaction', function() { describe('as an uncontrolled component', function() { + it('should default to selecting the first tab', function() { + const tabs = TestUtils.renderIntoDocument(createTabs()); + + assertTabSelected(tabs, 0); + }); + it('should update selected tab when clicked', function() { const tabs = TestUtils.renderIntoDocument(createTabs()); @@ -155,6 +187,7 @@ describe('react-tabs', function() { it('should call onSelect when a tab is clicked', function() { const called = {index: null}; const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 0, onSelect: function(index) { called.index = index; } @@ -167,6 +200,7 @@ describe('react-tabs', function() { it('should not call onSelect when a disabled tab is clicked', function() { const called = {index: null}; const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 0, onSelect: function(index) { called.index = index; } @@ -177,7 +211,10 @@ describe('react-tabs', function() { }); it('should not automatically update selected tab when clicked', function() { - const tabs = TestUtils.renderIntoDocument(createTabs({selectedIndex: 0})); + const tabs = TestUtils.renderIntoDocument(createTabs({ + selectedIndex: 0, + onSelect: () => {} + })); TestUtils.Simulate.click(findDOMNode(tabs.getTab(1))); assertTabSelected(tabs, 0); @@ -185,10 +222,16 @@ describe('react-tabs', function() { it('should update selected tab when selectedIndex changes', function() { const container = document.createElement('div'); - const tabs = render(createTabs({selectedIndex: 1}), container); + const tabs = render(createTabs({ + selectedIndex: 1, + onSelect: () => {} + }), container); assertTabSelected(tabs, 1); - render(createTabs({selectedIndex: 0}), container); + render(createTabs({ + selectedIndex: 0, + onSelect: () => {} + }), container); assertTabSelected(tabs, 0); unmountComponentAtNode(container); @@ -224,6 +267,24 @@ describe('react-tabs', function() { }); describe('validation', function() { + it('should result with warning when selectedIndex is present but onSelect is missing', function() { + const tabs = TestUtils.renderIntoDocument( + + ); + + const result = Tabs.propTypes.selectedIndex(tabs.props, 'children', 'Tabs'); + ok(result instanceof Error); + }); + + it('should result with warning when onSelect is present but selectedIndex is missing', function() { + const tabs = TestUtils.renderIntoDocument( + {}}/> + ); + + const result = Tabs.propTypes.onSelect(tabs.props, 'children', 'Tabs'); + ok(result instanceof Error); + }); + it('should result with warning when tabs/panels are imbalanced', function() { const tabs = TestUtils.renderIntoDocument( diff --git a/lib/helpers/onSelectPropType.js b/lib/helpers/onSelectPropType.js new file mode 100644 index 0000000000..cc797e3f30 --- /dev/null +++ b/lib/helpers/onSelectPropType.js @@ -0,0 +1,20 @@ +import React from 'react'; + +module.exports = function onSelectPropType(props) { + // onSelect is optional + if (!('onSelect' in props)) { + return null; + } + + // onSelect, if present, must be a function + const error = React.PropTypes.func.apply(null, arguments); + if (error) { + return error; + } + + if (typeof props.selectedIndex !== 'number') { + return new Error('`onSelect` must be accompanied by a numeric `selectedIndex`'); + } + + return null; +}; diff --git a/lib/helpers/selectedIndexPropType.js b/lib/helpers/selectedIndexPropType.js new file mode 100644 index 0000000000..a46446b083 --- /dev/null +++ b/lib/helpers/selectedIndexPropType.js @@ -0,0 +1,18 @@ +import React from 'react'; + +module.exports = function selectedIndexPropType(props) { + // selectedIndex is optional + if (!('selectedIndex' in props)) { + return null; + } + + // selectedIndex, if present, must be a number + const error = React.PropTypes.number.apply(null, arguments); + if (error) { + return error; + } + + if (typeof props.onSelect !== 'function') { + return new Error('`selectedIndex` must be accompanied by a function `onSelect`'); + } +};