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

[mui-system][style] bug fix for style value check color in nullable object #39457

Merged
merged 2 commits into from Oct 16, 2023
Merged

[mui-system][style] bug fix for style value check color in nullable object #39457

merged 2 commits into from Oct 16, 2023

Conversation

DarhkVoyd
Copy link
Contributor

Description
In PR #39071, we added checks to getStyleValue to throw a warning in development mode to double check if the prop value resolved from themeMapping was intentionally object. This was to ensure that setting object values to css such as color does not fail silently. But, the warning was also thrown on setting object values when necessary such as typography to body1 which quickly led to spam a lot of warnings.

Changes Made:

  • Added a function to check if a given string is a colour in any of the standard color styles (hex, rgb, rgba, var, scss).
  • Updated the getStyleValue function to check if the resolved value is non-null object and only throw warning if the object immediately contains color values.
  • Updated all tests from the previous PR to better fit the new getStyleValue.

Testing:

I have updated all relevant tests from the original PR, style.test.js, palette.test.js, styleFunctionsSx.test.js and Typography.test.js file to ensure that the changes work as expected. The tests include checking that the warning is only thrown on objects which immediately contain colours.

Closes #39399

@mui-bot
Copy link

mui-bot commented Oct 15, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 9695219

@brijeshb42 brijeshb42 merged commit 5c43c72 into mui:master Oct 16, 2023
19 checks passed
@brijeshb42 brijeshb42 added bug 🐛 Something doesn't work package: system Specific to @mui/system regression A bug, but worse and removed bug 🐛 Something doesn't work labels Oct 16, 2023
@DiegoAndai
Copy link
Member

Should we reopen #38478?

@DarhkVoyd
Copy link
Contributor Author

@DiegoAndai I do think we need to reopen but as of now I haven't been able to find a better way to solve it.

mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Oct 17, 2023
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 regression A bug, but worse
Projects
None yet
4 participants