Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/basic/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const App = React.createClass({
<Tab>React</Tab>
<Tab>Ember</Tab>
<Tab>Angular</Tab>

<span>+</span>
</TabList>

<TabPanel>
Expand Down
17 changes: 13 additions & 4 deletions src/components/TabList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,21 @@ import React, { PropTypes } from 'react';
import cx from 'classnames';

function renderChildren(props) {
return React.Children.map(props.children, (child) =>
React.cloneElement(child, {
return React.Children.map(props.children, (child) => {
// if child is not a tab we don't need to clone it
// since we don't need to add custom props

if (child.type.displayName !== 'Tab') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason to use the displayName. If not I think it would be better to do child.type !== Type and import Tab from './Tab' at the top.

return child;
}

const clonedProps = {
activeTabClassName: props.activeTabClassName,
disabledTabClassName: props.disabledTabClassName,
})
);
};

return React.cloneElement(child, clonedProps);
Copy link
Collaborator

@danez danez Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would still clone everytime.

    if (child.type.displayName === 'Tab') {
      return React.cloneElement(child, {
        activeTabClassName: props.activeTabClassName,
        disabledTabClassName: props.disabledTabClassName,
      });
    }

    return child;

});
}

module.exports = React.createClass({
Expand Down
18 changes: 11 additions & 7 deletions src/components/Tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -209,13 +209,17 @@ module.exports = React.createClass({

index++;

return cloneElement(tab, {
ref,
id,
panelId,
selected,
focus,
});
if (tab.type.displayName === 'Tab') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: tab.type === Tab

return cloneElement(tab, {
ref,
id,
panelId,
selected,
focus,
});
}

return tab;
}),
});

Expand Down
43 changes: 41 additions & 2 deletions src/components/__tests__/Tabs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,27 @@ describe('react-tabs', () => {
expect(wrapper.childAt(2).text()).toBe('Hello Bar');
expect(wrapper.childAt(3).text()).toBe('Hello Baz');
});

it('should not clone non tabs element', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to figure out how to test this 👍
You can just add /* eslint-disable react/no-multi-comp */ at the top of the test to disable the lint.

class Demo extends React.Component {
render() {
const plus = <div ref="yolo">+</div>;

return (<Tabs>
<TabList>
<Tab>Foo</Tab>
{plus}
</TabList>

<TabPanel>Hello Baz</TabPanel>
</Tabs>);
}
}

const wrapper = mount(<Demo />);

expect(wrapper.ref('yolo').text()).toBe('+');
});
});

describe('validation', () => {
Expand All @@ -201,7 +222,25 @@ describe('react-tabs', () => {
expect(result instanceof Error).toBe(true);
});

it('should result with a warning when wrong element is found', () => {
it(`should result with warning when tabs/panels are imbalanced and
it should ignore non tab children`, () => {
const wrapper = shallow(
<Tabs>
<TabList>
<Tab>Foo</Tab>
<div>+</div>
</TabList>

<TabPanel>Hello Foo</TabPanel>
<TabPanel>Hello Bar</TabPanel>
</Tabs>
);

const result = Tabs.propTypes.children(wrapper.props(), 'children', 'Tabs');
expect(result instanceof Error).toBe(true);
});

it('should not throw a warning when wrong element is found', () => {
const wrapper = shallow(
<Tabs>
<TabList>
Expand All @@ -213,7 +252,7 @@ describe('react-tabs', () => {
);

const result = Tabs.propTypes.children(wrapper.props(), 'children', 'Tabs');
expect(result instanceof Error).toBe(true);
expect(result instanceof Error).toBe(false);
});

it('should be okay with rendering without any children', () => {
Expand Down
4 changes: 0 additions & 4 deletions src/helpers/childrenPropType.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ module.exports = function childrenPropTypes(props, propName) {

if (c.type === Tab) {
tabsCount++;
} else {
error = new Error(
`Expected 'Tab' but found '${c.type.displayName || c.type}'`
);
}
});
} else if (child.type.displayName === 'TabPanel') {
Expand Down