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

[infra] Automatically get info for CSS variables #874

Merged
merged 26 commits into from
Dec 4, 2024

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Nov 26, 2024

This PR proposes a more structured way of defining CSS variables used in the components. The benefits are:

  • avoid typos, we need to review only one place for checking if the variables are written correctly
  • we can extract automatically the info for the CSS variables and use it in the API table (this was the main goal for proposing the change)

Depends on mui/material-ui#44587

Preview: https://deploy-preview-874--base-ui.netlify.app/new/components/dialog#popup

I will update the rest of the CSS variables usages in a follow up PR.

@mui-bot
Copy link

mui-bot commented Nov 26, 2024

Netlify deploy preview

https://deploy-preview-874--base-ui.netlify.app/

Generated by 🚫 dangerJS against 5b49e7c

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

👼

packages/react/src/accordion/cssVars.ts Outdated Show resolved Hide resolved
packages/react/src/accordion/cssVars.ts Outdated Show resolved Hide resolved
@@ -20,11 +20,5 @@
"type": "number",
"description": "Indicates how many dialogs are nested within."
}
},
"cssVariables": {
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for manually defining it 🎉

package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,11 @@
export type DialogPopupCssVars = {
Copy link
Member Author

Choose a reason for hiding this comment

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

The convention here is tha the type with [ComponentName]CssVars exists. By default, the type is assumed string, if it is something else we can add the @type tag. The actual CSS variable in the docs is the kebab case of the key. I added support for the @deprecated tag too, in case we ever need it in the future.

nestedDialogs: string;
};

export const cssVars: DialogPopupCssVars = generateCssVariables(['nestedDialogs']);
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 added this util similar to the utility for generating classes we have in other projects: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Dialog/dialogClasses.ts#L43, to make sure we don't have typos while generating the CSS variables.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@mnajdova mnajdova marked this pull request as ready for review November 27, 2024 14:34
@mnajdova mnajdova requested review from michaldudak and vladmoroz and removed request for atomiks November 27, 2024 14:35
@zannager zannager added the scope: infra Org infrastructure work going on behind the scenes label Nov 29, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 2, 2024
@mnajdova mnajdova mentioned this pull request Dec 3, 2024
1 task
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 4, 2024
@mnajdova mnajdova merged commit e7a245c into mui:master Dec 4, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: infra Org infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants