[core] Ensure component class keys aren't missing#25754
Conversation
|
Details of bundle changes (experimental) @material-ui/core: parsed: +0.09% , gzip: +0.05% |
mnajdova
left a comment
There was a problem hiding this comment.
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.
oliviertassinari
left a comment
There was a problem hiding this comment.
It looks great. So it's a follow-up on #24736, it seems we push option 1 further.
|
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. |
ab7f303 to
6ace5a0
Compare
294d3e0 to
76f3f74
Compare
1568c0e to
62684a8
Compare
mnajdova
left a comment
There was a problem hiding this comment.
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.
For review I would suggest going by commits and ignore
ff08701(#25754) and instead review https://gist.github.com/eps1lon/4326159c3968e74548b757cb9c69ccd2 which generatedff08701(#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 ingenerateUtilityClassesB. Make sure that all classes in
generateUtilityClassesare listed inProps['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-activewould not be documented.Benefits:
The classes documentation is now available on hover when working in the component implementation:
The public
*Classes(e.g.AccordionClasses) type has now the same JSDOC has theclassesprop.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