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.

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

[core] Ensure component class keys aren't missing #25754

Merged
merged 11 commits into from
May 15, 2021

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 13, 2021

For review I would suggest going by commits and ignore ff08701 (#25754) and instead review https://gist.github.com/eps1lon/4326159c3968e74548b757cb9c69ccd2 which generated ff08701 (#25754)

The goal is to make sure that Props['classes'] lists all the classes automatically that are used. Unfortunately we can only decide on one:
A. Make sure that all properties in Props['classes'] are listed in generateUtilityClasses
B. Make sure that all classes in generateUtilityClasses are listed in Props['classes']

We can't ensure an exact match (as far as I can tell1) but at least we can make sure that one is a subset of another. I choose A since that allows us to have "private" classes e.g. in const classes: { root: string } = generateUtilityClasses('MuiComponent', ['root', 'active']) MuiComponent-active would not be documented.

Benefits:

The classes documentation is now available on hover when working in the component implementation:
classes-intellisense

The public *Classes (e.g. AccordionClasses) type has now the same JSDOC has the classes prop.

I'm also going to remove the JSDOC class documentation from the implementation in a follow-up (for git-blame). We no longer verify these match with the documentation and the JSDOC in the implementation was already outdated. They're still either on hover or one file away. A missing comment is better than a misleading one in the end.

1TypeScript playground for the proposal

@eps1lon eps1lon added typescript package: material-ui Specific to @mui/material labels Apr 13, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 13, 2021

Details of bundle changes (experimental)

@material-ui/core: parsed: +0.09% , gzip: +0.05%

Generated by 🚫 dangerJS against b79c297

@eps1lon eps1lon changed the title [unstyled] Convert generateUtilityClass(es) to TypeScript [core] Ensure component class keys aren't missing Apr 13, 2021
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The goal is to make sure that Props['classes'] lists all the classes automatically that are used. Unfortunately we can only decide on one:
A. Make sure that all properties in Props['classes'] are listed in generateUtilityClasses
B. Make sure that all classes in generateUtilityClasses are listed in Props['classes']
We can't ensure an exact match (as far as I can tell1) but at least we can make sure that one is a subset of another. I choose A since that allows us to have "private" classes e.g. in const classes: { root: string } = generateUtilityClasses('MuiComponent', ['root', 'active']) MuiComponent-active would not be documented.

Agree, for choosing A.

packages/material-ui/src/Accordion/Accordion.js Outdated Show resolved Hide resolved
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It looks great. So it's a follow-up on #24736, it seems we push option 1 further.

packages/material-ui/src/Accordion/Accordion.js Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member Author

eps1lon commented Apr 14, 2021

Alright, thanks for the feedback. Looks like it's pretty much uncontroversial so I'll go ahead and apply this in all relevant places over the next week.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 13, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 14, 2021
@eps1lon eps1lon marked this pull request as ready for review May 14, 2021 11:08
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks great, I like how you were able to find missing classes and fix them 👍 @siriwatknp to be aware of these changes, for the migration PRs that are coming up.

@eps1lon eps1lon merged commit 623c59d into mui:next May 15, 2021
@eps1lon eps1lon deleted the feat/typed-classes branch May 15, 2021 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants