fix(theme-classic): allow tabs with number as value #5732
Conversation
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
|
✔️ [V2] 🔨 Explore the source changes: fb8e7cf 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6170c360b6aa780008f865a6 😎 Browse the preview: https://deploy-preview-5732--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5732--docusaurus-2.netlify.app/ |
| throw new Error( | ||
| `Docusaurus error: Bad <Tabs> child <${ | ||
| // @ts-expect-error: guarding against unexpected cases | ||
| typeof child.type === 'string' ? child.type : child.type.name |
There was a problem hiding this comment.
Error: Docusaurus error: Bad child : all children of the component should be , and every should have a unique "value" prop.
=> should not print "undefined", maybe child.type.name is wrong or does not work in all cases?
There was a problem hiding this comment.
This I'm actually not sure. I think it's because they're using the staging preview which is prod, so the name info is gone.
packages/docusaurus-theme-classic/src/theme/Tabs/__tests__/index.test.tsx
Outdated
Show resolved
Hide resolved
| @@ -37,24 +25,11 @@ function TabsComponent(props: Props): JSX.Element { | |||
| groupId, | |||
| className, | |||
| } = props; | |||
| const children = Children.map(props.children, (child) => { | |||
| if (isValidElement(child) && isTabItem(child)) { | |||
There was a problem hiding this comment.
IMHO the real problem is in this test: it should pass but for some reason doesn't. Wonder what is child. I expect it to be a tab but maybe React does a weird thing and gives an array?
Maybe use Children.toArray at the beginning?
There was a problem hiding this comment.
Note this might break because the user is using dynamic tabs with a single element in the array.
Children.map may return a single mapped element here, instead of an array
There was a problem hiding this comment.
I actually don't know how Children.toArray or Children.map operates. I suppose you know more about React than I do🤪 Any materials that cover this?
There was a problem hiding this comment.
Ugh, it's because of a very dumb thing: their value is a number, not a string.
https://github.com/mit-dormcon/website/blame/broken-tabs/docs/archive.mdx#L19
I think we should just widen the check to typeof comp.props.value !== 'undefined'. Thanks for compelling me to look deeper into this😄
There was a problem hiding this comment.
ah 😄 great you could find the cause
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
Josh-Cena
changed the title
|
LGTM 👍 |
Motivation
Fix #5729. I'm not sure if we should remove the check for
valueentirely, but let's just trust the users more since we don't know what they will present.Edit. The issue is just caused by a too strict type check. Widening the checking solved it.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Fixed test cases