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

[system] Support CSS grey color in sx #34548

Merged
merged 7 commits into from
Oct 12, 2022
Merged

Conversation

TheUnlocked
Copy link
Contributor

Issues

Changes

When "grey" is used as the value for a color prop in sx, a warning will now be printed to the console explaining the issue and a number of potential remedies.

Resolves #31203

@mui-bot
Copy link

mui-bot commented Oct 1, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 2218572

@siriwatknp
Copy link
Member

siriwatknp commented Oct 4, 2022

🤔 I am opposed to displaying warnings. It should work first with native grey and then lookup to the theme.

For me, showing warnings might not be a good experience since using color: grey is valid in CSS.

@TheUnlocked
Copy link
Contributor Author

@siriwatknp Adjusted to just use grey when the user enters "grey".

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Could you add a test to verify?

@siriwatknp siriwatknp added the PR: needs test The pull request can't be merged label Oct 5, 2022
@siriwatknp siriwatknp changed the title [system] Add warning when "grey" is used for color prop in sx [system] support CSS grey color in sx Oct 5, 2022
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for your contribution! cc @mnajdova for another review because my suggestion changes the initial solution.

@siriwatknp siriwatknp added package: system Specific to @mui/system and removed PR: needs test The pull request can't be merged labels Oct 7, 2022
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Looks better than what I proposed in the issue. Good job!

@mnajdova mnajdova changed the title [system] support CSS grey color in sx [system] Support CSS grey color in sx Oct 12, 2022
@mnajdova mnajdova merged commit 4a69985 into mui:master Oct 12, 2022
alexfauquette pushed a commit to alexfauquette/material-ui that referenced this pull request Oct 14, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@romgrk
Copy link
Contributor

romgrk commented Nov 1, 2024

This introduces an unpleasant performance cost, all colors that go through sx need to pass through that function. I wonder if there would be a different way to solve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[system] grey is no more recognized as color with the sx prop
5 participants