[Badge] Fix TS custom variants #24407
Conversation
oliviertassinari
added
component: badge
docs/pages/api-docs/badge.json
Outdated
| }, | ||
| "default": "'standard'" | ||
| }, | ||
| "variant": { "type": { "name": "string" }, "default": "'standard'" }, |
There was a problem hiding this comment.
This doesn't seem intended. We now say Badge expects any string when it only supports 'dot' | 'standard': https://deploy-preview-24407--material-ui.netlify.app/api/badge/
Though it's already not ideal on next: https://next--material-ui.netlify.app/api/badge/
It should just say 'dot' | 'standard', no?
There was a problem hiding this comment.
Just to clarify: The goal of this PR doesn't need to be 'dot' | 'standard'. Keeping the current one is fine.
There was a problem hiding this comment.
I can take a look later on whether we can improve the generation of the docs regarding these kind of props.
There was a problem hiding this comment.
I can take a look later on whether we can improve the generation of the docs regarding these kind of props.
When is this later going to be? We already made it worse when introducing string (#22140). Now we're making it worse again. Follow-ups are ok but it's clear to me that they're not working in this instance.
There was a problem hiding this comment.
I think that the expected result in the docs is "string" for the unstyled component. It's meant to not create any constraints when implementing a design spec. For the styled component, it does look like a regression.
There was a problem hiding this comment.
We already made it worse when introducing string (#22140).
worse better smile
The documentation got worse. Badge#variant does not support arbitrary strings which the change in #22140 implied.
Expected:
Badge#variantsupports'dot' | 'standard'UnstyledBadge#variantsupportsstring
Actual:
Badge#variantsupportsstring(was wrong before with'dot' | 'standard' | string)
Only /api/badge is problematic. The types seem correct. They still reject an unsupported variant: a6e460c (#24407)
There was a problem hiding this comment.
I see now, let me take a look on buildApi and what we have there. I think the string may be coming from the unstyled component.
There was a problem hiding this comment.
I've fixed the issue in the buildAPI, there are no changes to the /api/badge now. For not including the | string part, let's resolve it separately. I think it will require more work, or we will need to whitelist which props should have a custom documentation logic, so we can extract the string option out of those (variant, color, size and all other props for which we want to support custom defined values)
Closes #24323