-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[code-infra] Refined docs generation #40603
Conversation
Netlify deploy previewhttps://deploy-preview-40603--material-ui.netlify.app/ Bundle size report |
As I noted in mui/mui-x#11538 (comment), I think we should treat the "root" slot in a special way and place it at the top, even if the others are sorted alphabetically. @samuelsycamore, @danilo-leal, what do you think? |
I'd agree — having the classes sorted closely to how we write the CSS itself would be better/easier to parse, in my opinion. I personally usually do root → any child styles → hover → focus → active. 👍 |
Notice that classes are not concerned by the modification since they are already sorted by alphabetical order. Only slots are modified in this PR. Because classes were already sorted: material-ui/packages/api-docs-builder/utils/parseSlotsAndClasses.ts Lines 64 to 66 in 8cc116a
For slots, my proposal would be to add a config such that core can keep slots order, and on x sort them the way we want |
Another argument for sorting - they are sorted alphabetically on the IDE suggestions list. 😉 |
Few learning while doing that PR:
@michaldudak The second point explain why lot of |
const nonSlotClassDefinitions = classDefinitions.filter( | ||
(classDefinition) => !Object.keys(slots).includes(classDefinition.key), | ||
); |
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'm removing the sorting logic here such that sorting for classes and slots are both at the same place
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.
It does work well. I found a few grammar and spelling mistakes, but I'm approving it already.
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Alexandre Fauquette <[email protected]>
This PR contains two modifications. I recomand reviewing per commit.
It sorts
slots
andclasses
item array alphabetically according to their names. For classes, seems it was already the case.got added because of this comment: [DataGrid] Allow to filter non-filterable columns programmatically mui-x#11538 (comment)Not sure, but I suspect the slots to be reliable on core because they are mostly defined in a single place whereas X has more interconnected slots, and so the order they get process might depend on the order TS resolves slots typing.
To update monorepo on v6 and still be able to parse slots, I needed to add a way to customize how slots are found because v6 does not use the same strategy as base for slot type naming: Bump monorepo mui-x#11667
I can still see some inconsistencies in object keys order (see this comment) Not sure about how we could ensure a consistency on this aspect.