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.

Sign up for GitHub

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

[Badge] Fix TS custom variants #24407

Merged
merged 6 commits into from
Jan 14, 2021
Merged

[Badge] Fix TS custom variants #24407

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e2538c
relaxed unstyled variant prop type
mnajdova Jan 14, 2021
f28af7b
added module augmentation test
mnajdova Jan 14, 2021
bf0a18c
prettier
mnajdova Jan 14, 2021
a6e460c
add true negative
eps1lon Jan 14, 2021
36de6e3
fixed regression on the badge
mnajdova Jan 14, 2021
b6967d0
prettier
mnajdova Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions docs/pages/api-docs/badge-unstyled.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,7 @@
"default": "'rectangular'"
},
"showZero": { "type": { "name": "bool" } },
"variant": {
"type": {
"name": "union",
"description": "'dot'<br>&#124;&nbsp;'standard'<br>&#124;&nbsp;string"
},
"default": "'standard'"
}
"variant": { "type": { "name": "string" }, "default": "'standard'" }
},
"name": "BadgeUnstyled",
"styles": {
Expand Down
8 changes: 1 addition & 7 deletions docs/pages/api-docs/badge.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,7 @@
},
"showZero": { "type": { "name": "bool" } },
"sx": { "type": { "name": "object" } },
"variant": {
"type": {
"name": "union",
"description": "'dot'<br>&#124;&nbsp;'standard'<br>&#124;&nbsp;string"
},
"default": "'standard'"
},
"variant": { "type": { "name": "string" }, "default": "'standard'" },
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify: The goal of this PR doesn't need to be 'dot' | 'standard'. Keeping the current one is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can take a look later on whether we can improve the generation of the docs regarding these kind of props.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eps1lon eps1lon Jan 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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#variant supports 'dot' | 'standard'
  • UnstyledBadge#variant supports string

Actual:

  • Badge#variant supports string (was wrong before with 'dot' | 'standard' | string)

Only /api/badge is problematic. The types seem correct. They still reject an unsupported variant: a6e460c (#24407)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

"component": { "type": { "name": "elementType" } }
},
"name": "Badge",
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export interface BadgeUnstyledTypeMap<P = {}, D extends React.ElementType = 'div
* The variant to use.
* @default 'standard'
*/
variant?: 'standard' | 'dot';
variant?: string;
};
defaultComponent: D;
}
Expand Down
5 changes: 1 addition & 4 deletions packages/material-ui-unstyled/src/BadgeUnstyled/BadgeUnstyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,7 @@ BadgeUnstyled.propTypes = {
* The variant to use.
* @default 'standard'
*/
variant: PropTypes /* @typescript-to-proptypes-ignore */.oneOfType([
PropTypes.oneOf(['dot', 'standard']),
PropTypes.string,
]),
variant: PropTypes.string,
};

export default BadgeUnstyled;