Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added nested Tablist Support #180

Closed
wants to merge 2 commits into from
Closed

Conversation

McPo
Copy link

@McPo McPo commented May 11, 2017

You can now nest TabList as per #114

ie.

<Tabs>
  <div id="tabs-nav-wrapper">
    <button>Left</button>
    <div className="tabs-container">
      <TabList>...</TabList>
    </div>
    <button>Right</button>
  </div>
  {tabPanels}
</Tabs>

Few things

  • As its recursive, it could be slow depending on the complexity of the nodes. (Should this be a switch, or should we define the tablist via refs or something?)
  • I have removed a check within propTypes.js, as it no longer makes sense.
  • As such, I think we should just call getTabsCount and getPanelsCount, as opposed to duplicating code.
  • The 2nd filter in getTabsCount is now redundant, we could just count the tabs directly
  • Haven't tested nested TabPanel (This could probably be added with minimal effort)
  • react-children-utilities has a deepFind feature as opposed to using deepForEach, however it doesn't appear to search all nodes (due to a toArray)

Thoughts?

@tengis
Copy link

tengis commented May 27, 2017

Much needed feature +1

@MindRave
Copy link

Much needed feature indeed, and nested TabPanels are much needed as well. There's simply no reason for not being able to wrap a TabPanel in a div, for example.

Copy link
Collaborator

@danez danez left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. As commented inline this will break nesting of react-tabs right now. Maybe you have an idea how to work around this?

@@ -132,7 +133,7 @@ export default class UncontrolledTabs extends Component {
}

// Map children to dynamically setup refs
return React.Children.map(children, child => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will not work when another react-tabs instance is nested inside I think, as it will find all children.

<Tabs>
<TabPanel>
  <Tabs>
  <TabPanel></TabPanel>
  </Tabs>
</TabPanel>
</Tabs>

Not sure how to avoid this, as we also couldn't check if child === Tabs as children seem to be visited first.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also the reason why the tests are failing.

@@ -157,7 +158,7 @@ export default class UncontrolledTabs extends Component {
}

result = cloneElement(child, {
children: React.Children.map(child.props.children, tab => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When we use deepMap, we could actually remove the nested map here, as we would visit the Tab anyway in the outer map?

import TabList from '../components/TabList';
import Tab from '../components/Tab';
import TabPanel from '../components/TabPanel';

export function getTabsCount(children) {
const tabLists = React.Children.toArray(children).filter(x => x.type === TabList);
let tabList;
Children.deepForEach(children, c => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use deepFind here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see your comment now in the inital message...hmm

error = new Error(
`Expected 'TabList' or 'TabPanel' but found '${child.type.displayName || child.type}' in \`${componentName}\``,
);
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes this could be removed then.

@danez
Copy link
Collaborator

danez commented Jun 13, 2017

Merged in #180 and 430ed30

@danez danez closed this Jun 13, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants