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

[material-ui][Typography] Color prop check for primitive type #39071

Merged
merged 8 commits into from
Oct 5, 2023
Merged

[material-ui][Typography] Color prop check for primitive type #39071

merged 8 commits into from
Oct 5, 2023

Conversation

DarhkVoyd
Copy link
Contributor

Changes Made:

  • Updated the getStyleValue function to check if the provided color value is a primitive type (string, number, boolean, etc.) before setting the property's value.
  • If the provided color value is not a primitive type, the function will not set the property's value and will log an error to the console in development mode.
  • Added tests to ensure the correctness of the generated CSS when using invalid color values.

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

Sorry, something went wrong.

@mui-bot
Copy link

mui-bot commented Sep 20, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 088a77d

@danilo-leal danilo-leal changed the title [material] Typography color prop check for primitive type [material-ui][Typography] Color prop check for primitive type Sep 20, 2023
@danilo-leal danilo-leal added typescript component: Typography The React component. package: material-ui Specific to @mui/material labels Sep 20, 2023
@DarhkVoyd
Copy link
Contributor Author

@brijeshb42 Can you please help me with these failed checks?

@DarhkVoyd
Copy link
Contributor Author

@brijeshb42 kindly review these changes.

@brijeshb42
Copy link
Contributor

@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

@DarhkVoyd
Copy link
Contributor Author

Sure, I am looking at the failing tests.

@DarhkVoyd
Copy link
Contributor Author

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

@brijeshb42
Copy link
Contributor

Seems like an error failing on master as well. You'll have to rebase when it's fixed on master.

Comment on lines 120 to 131
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();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

@DarhkVoyd DarhkVoyd Sep 28, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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".`,
);
}
Copy link
Contributor

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.

Copy link
Contributor Author

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

@DarhkVoyd
Copy link
Contributor Author

@brijeshb42 I have made necessary changes, kindly tell me when to rebase.

@brijeshb42
Copy link
Contributor

@DarhkVoyd Can you rebase and push again? The build should pass now.

@DarhkVoyd
Copy link
Contributor Author

@brijeshb42 Yes, the build passes now.

@brijeshb42 brijeshb42 merged commit 47eaaa0 into mui:master Oct 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Typography The React component. package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material] Invalid color prop has no effect
4 participants