Closed jodyheavener closed 1 year ago
I thought about that; but is there a legitimate use-case for a single tab children? I get your point that the type of props serve two purposes: (a) tell the component itself what it's going to receive at runtime (b) tell the users what they should pass to the component. Here we are trying to be restrictive for the second use-case. The point of types is to provide a strict contract and catch user mistakes as early as possible.
IMO the real problem is that tabs are mostly used in MDX where there's no type-checking, so this works just as well:
<Tabs />
But I don't think that warrants adding null
to children
, because that's not a legitimate state either. What do you think?
is there a legitimate use-case for a single tab children?
If it helps, our use-case is that we've got a page breaking down the compatibility of our software with other software, and each section can have one or more operating systems in a tabbed layout. In some cases there is only one OS. There is arguably a better approach for this, but this actually does a nice job when it comes to layout uniformity and synchronizing instructions based on the chosen OS.
I agree that this is made harder by that fact that there's no type-checking in MDX, but the important thing IMO is that it still does render when null
, a single element, or an array of them, which sets an expectation that children
should be typed as such since there is no runtime check (i.e. why I thought this was a bug).
Let's add a runtime transformation to ensure it's always an array.
I did a bit more research into this and realized that we can't transform the received children before it gets to the Tabs
component (or in my case, the wrap-swizzled version), that children being multiple types is a performance thing, and that the theme-classic component already correctly uses React.Children.map
😄 So I think the only thing to do here is to update the type - possibly from ReactElement<TabItemProps>[]
to ReactNode
?
Have you read the Contributing Guidelines on issues?
Prerequisites
npm run clear
oryarn clear
command.rm -rf node_modules yarn.lock package-lock.json
and re-installing packages.Description
Discovered while swizzling the
Tabs
component, it appears the type forProps['children']
isReactElement<TabItemProps>[]
.This is largely true, except for when the
Tabs
component only has oneTabItem
child, in which case it is no longer an array; it's justReactElement<TabItemProps>
.So we should either coerce the individual
ReactElement
into an array, or update the interface to correctly reflect all valid types forchildren
.Reproducible demo
https://stackblitz.com/edit/github-pv7ajd?file=src%2Ftheme%2FTabs%2Findex.tsx,docs%2Ftabs-demo.mdx
Steps to reproduce
Two ways to test this. First, open the demo link provided.
In the browser:
yarn start
), navigate to/docs/tabs-demo
In the terminal:
yarn build
to build the site staticallyExpected behavior
According to the type for
Props["children"]
both logs should betrue
.Actual behavior
Tabs
component, which has multipleTabItem
component children, istrue
.Tabs
component, which has a singleTabItem
component child, isfalse
.Your environment
No response
Self-service