-
-
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
[material-ui][Typography] Color prop check for primitive type #39071
Conversation
Netlify deploy previewhttps://deploy-preview-39071--material-ui.netlify.app/ Bundle size report |
@brijeshb42 Can you please help me with these failed checks? |
@brijeshb42 kindly review these changes. |
@DarhkVoyd Can you take a look at the failing tests here and make sure they or fix the tests accordingly - https://circleci.com/gh/mui/material-ui/587030 https://circleci.com/gh/mui/material-ui/587028 |
Sure, I am looking at the failing tests. |
@brijeshb42 I have made changes to getStyleValue, added some test cases and resolved other failed checks. I am unable to understand this failed test because running npm release:build script works fine locally. |
Seems like an error failing on master as well. You'll have to rebase when it's fixed on master. |
render( | ||
<Typography variant="h6" color="background"> | ||
Hello | ||
</Typography>, | ||
); | ||
|
||
sinon.assert.calledWith( | ||
consoleWarnStub, | ||
'MUI: The value found in theme for prop: "background" is an [Object] instead of string or number. Check if you forgot to add the correct dotted notation, eg, "background.paper" instead of "background".', | ||
); | ||
|
||
consoleWarnStub.restore(); |
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.
render( | |
<Typography variant="h6" color="background"> | |
Hello | |
</Typography>, | |
); | |
sinon.assert.calledWith( | |
consoleWarnStub, | |
'MUI: The value found in theme for prop: "background" is an [Object] instead of string or number. Check if you forgot to add the correct dotted notation, eg, "background.paper" instead of "background".', | |
); | |
consoleWarnStub.restore(); | |
expect(() => render( | |
<Typography variant="h6" color="background"> | |
Hello | |
</Typography>, | |
)).toWarnDev('MUI: The value found in theme for prop: "background" is an [Object] instead of string or number. Check if you forgot to add the correct dotted notation, eg, "background.paper" instead of "background".') |
No need to use sinon. Same change in other place as well.
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 am unable to pass any of the warn tests this way, I get similar errors for each one of them:
<Typography /> prop: color should check for invalid color value FAILED
AssertionError: Recorded unexpected console.warn calls:
- Expected no more error messages but got:
"MUI: The value found in theme for prop: "background" is an [Object] instead of string or number. Check if you forgot to add the correct dotted notation, eg, "background.paper" instead of "background"."
I am unable to debug this.
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.
Here'e the changes I did - https://github.com/mui/material-ui/compare/master...brijeshb42:material-ui:issue-38478?expand=1
Basically, you add to call toWarnDev([msg, msg])
expecting the warn to be called twice. That's why it was failing.
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.
how can I merge these changes? kindly tell if anything else remaining to work on this.
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.
You can just copy-paste the toWarnDev
related changes to your branch. Nothing else after that.
console.warn( | ||
`MUI: The value found in theme for prop: "${propValueFinal}" is an [Object] instead of string or number. Check if you forgot to add the correct dotted notation, eg, "background.paper" instead of "background".`, | ||
); | ||
} |
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.
Please also set the userValue
to value
here like you were doing last time. If a proper value is not found in theme, it should just use whatever user provided in prop.
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.
Actually, it was removed on purpose because there are cases when value
could be an object such as Typography theme key body1 or h1 which is assigned as an object through handlebreakpoints. So, I think just warning the user for possible mistake is enough. If my point is unclear kindly refer to this test case
@brijeshb42 I have made necessary changes, kindly tell me when to rebase. |
@DarhkVoyd Can you rebase and push again? The build should pass now. |
@brijeshb42 Yes, the build passes now. |
Changes Made:
getStyleValue
function to check if the provided color value is a primitive type (string, number, boolean, etc.) before setting the property's value.Testing:
I have added tests to the
Typography.test.js
file to ensure that the changes work as expected. The tests include checking the correctness of generated CSS when using invalid color values.Closes #38478