Tabs/Tab component enhancements

[joshblack]

We should schedule some time to refactor/update the tab components with the following constraints in mind:

• Should support keyboard navigation, related to: feat(tabs): support home/end keyboard navigation #3258
• Could be updated to hooks to help address getDerivedStateFromProps
• Should keep in mind that we're using it in MDX and test with null and wrapped children accordingly
  • Specifically, renderContent as a defaultProp may not be properly hoisted and is causing issues within MDX

[vpicone]

At some point between 10.2 and 10.3 a regression caused the tabs content to not properly render, the result being an undefined element. In my opinion, special care should be used around the React helpers. They'll be required here to add an index to the tabs.

However, I think the top level helpers should likely be avoided if using JSX is possible, as mixing cloneElement, with JSX, with mapping over children can make the component harder to reason about.

In summary,

This code worked with 7.2

export const Tabs = props => (
  <CarbonTabs
    className={tabContainer}
    tabContentClassName={tabContents}
    {...props}
  />
);

This code is required for 7.3+

export const Tabs = props => (
  <CarbonTabs className={tabContainer} tabContentClassName={tabContents}>
    {React.Children.map(props.children, child =>
      React.cloneElement(child, {
        renderContent: TabContent,
      })
    )}
  </CarbonTabs>
);

[joshblack]

@vpicone could you provide more info to help with isolating the issue? renderContent specifically seems to be caused by usage with MDX, as by default it is supplied under defaultProps on the Tab component.

[vpicone]

@joshblack I'm not sure, only the props set in the cloneElement call are making it to the component. Seems like we could address this using the TabContent as a default in the Tabs component and not relying on default props of Tab? Seems like there's some discrepancy in when default props are loaded for create and clone element in react but I'm not sure if that has any relevancy.

[vpicone]

All things equal (mdx/gatsby/other deps) Tabs worked in 7.2 but not 7.3+ so I don't think we can blame MDX here but I could be wrong.

[joshblack]

@vpicone I'm not suggesting MDX is to blame, I'm requesting that we offer more details to help reproduce and fix the issue. This seems to be coming up when used alongside MDX, but if this is an issue outside of this scope then that's totally fine 👍 As long as there is a way for someone working on this to reproduce, identify, and fix the issue.

[stale]

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

[stale]

As there's been no activity since this issue was marked as stale, we are auto-closing it.