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

[code-infra] Refined docs generation #40603

Merged
merged 13 commits into from
Jan 23, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jan 15, 2024

This PR contains two modifications. I recomand reviewing per commit.

  1. It sorts slots and classes 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.

  2. 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.

@mui-bot
Copy link

mui-bot commented Jan 15, 2024

Netlify deploy preview

https://deploy-preview-40603--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 3577970

@alexfauquette alexfauquette added the scope: code-infra Specific to the core-infra product label Jan 15, 2024
@alexfauquette alexfauquette marked this pull request as ready for review January 15, 2024 11:10
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2024
@michaldudak
Copy link
Member

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?

@danilo-leal
Copy link
Contributor

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. 👍

@alexfauquette
Copy link
Member Author

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:

const nonSlotClassDefinitions = classDefinitions
.filter((classDefinition) => !Object.keys(slots).includes(classDefinition.key))
.sort((a, b) => a.key.localeCompare(b.key));

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

@LukasTy
Copy link
Member

LukasTy commented Jan 18, 2024

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. 😉

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2024
@alexfauquette
Copy link
Member Author

alexfauquette commented Jan 18, 2024

Few learning while doing that PR:

  • Don't forget sorting are mutating the array. (it led to some weird behavior multiple time)
  • I sorted array while generating the translation objects, because it ensure the object key order which was flaky from time to time

@michaldudak The second point explain why lot of "slotDescriptions" are modified (reordered) I made sure they are effectively just reorder

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 19, 2024
Comment on lines +66 to +68
const nonSlotClassDefinitions = classDefinitions.filter(
(classDefinition) => !Object.keys(slots).includes(classDefinition.key),
);
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'm removing the sorting logic here such that sorting for classes and slots are both at the same place

Copy link
Member

@michaldudak michaldudak left a 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.

packages/api-docs-builder/ProjectSettings.ts Outdated Show resolved Hide resolved
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
@alexfauquette alexfauquette merged commit 531646c into mui:master Jan 23, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants