-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[docs-infra] Simplify CSS classes extraction in API docs generator #39808
[docs-infra] Simplify CSS classes extraction in API docs generator #39808
Conversation
Netlify deploy previewhttps://deploy-preview-39808--material-ui.netlify.app/ Bundle size report |
1192007
to
ad741a8
Compare
ad741a8
to
04ed3d5
Compare
04ed3d5
to
93d34a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much more readable like that.
I reviewed the docs part. Still need some time to review in depth the new JSON generation process, and compare the preview with the prod
const result: { classNames: string[]; descriptions: Record<string, string> } = { | ||
classNames: [], | ||
descriptions: {}, | ||
muiName: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to use an enum here? Otherwise it's a pain when we navigate this kind of code and don't know exactly which convention is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give an example? Where exactly would you see an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood what this property is about. I thought it was storing the mui project (core, base, joy, ...) but it's the component name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's the name of the component used in style overrides object (such as MuiButton
).
...getClassesToC({ | ||
t, | ||
componentName: pageContent.name, | ||
componentStyles, | ||
componentClasses, | ||
}), | ||
componentSlots?.length > 0 && createTocEntry('slots'), | ||
hasClasses && createTocEntry('classes'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this modification, you will get a ToC with
- classes (expanded)
- slots
- classes
Should replace the hasClasses && createTocEntry('classes'),
by ...getClassesToC({ })
to respect the page order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
It won't actually ever call createTocEntry('classes')
because I introduced a bug in hasClasses
;)
const options: ApiDisplayOptions[] = ['collapsed', 'expanded', 'table']; | ||
const DEFAULT_LAYOUT: ApiDisplayOptions = 'expanded'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This typo fix might need extra work.
In getOption
and getRandomOption
the loclastorage can still return 'expended'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, indeed. I'll make sure it uses a default value when local storage has something unknown.
Nice work. A bit related to this, I've created this issue: #39867 we should not show the rule name for the state classes as you can't override them without bumping the specificity. |
@alexfauquette, I'd appreciate if you could take a look at the remaining changes. I need this PR to work on #39467 further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Currently we have mostly duplicated logic for CSS classes extraction in API docs builder and two similar fields in the resulting API description JSON files:
styles
andclasses
. This PR unifies the processing of CSS classes and gets rid of thestyles
field.The format of the
classes
field was also changed to make it easier to use in the docs: theclasses
andglobalClasses
fields inside theclasses
object were changed to an object with several properties:old:
new:
(while the
description
field isn't strictly necessary here, it was added to match props definitions)The docs were adapted to use this new format.
Another change was moving the responsibility of figuring out the actual class name to the API docs builder. Previously, we had logic for this in many different places - in some cases the
.Mui-
prefix was added in the docs, in other in the docs builder. With this change the docs will display the class name exactly as it was defined in the JSON file, without adding any prefixes. This will allow to configure how classes are generated in the API docs builder. This was my primary motivation for this PR, as I need to customize the prefix of Base UI CSS classes.This PR does not introduce changes visible to our users (except for a couple of corrected strings).
I recommend reviewing it by commit.
Fixes #39867