-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
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
[codemod] Add sx
prop for v6 migration
#42153
Conversation
Netlify deploy previewhttps://deploy-preview-42153--material-ui.netlify.app/ Bundle size report |
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.
Some initial feedback and thoughts.
|
||
<Box | ||
style={{ | ||
...props.style, |
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.
Shouldn't this come last?
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.
No, the generated CSS vars should come last to ensure that it takes effect. It'd be 99.9999% that the CSS vars from props.style
will get overridden.
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 don't understand the argument honestly. Regardless of what the sx
props generated, the users's style attribute should win, even if we almost never have cases where people need to do 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.
I'm fine to change. As a note, it does not matter which way we choose to override (before or after) because if the generated variable is the same as their existing style.*
, the result styles will not be the same but as I said, that's 99.9999%.
For example,
Before:
<Box
style={{
'--items-imageLight': '10px', // existing user's defined variable
}}
sx={{
fontSize: 'var(--items-imageLight)',
backgroundImage: (theme) =>
theme.palette.mode === 'light'
? items[selectedItemIndex].imageLight
: items[selectedItemIndex].imageDark,
}}
/>;
After:
<Box
style={{
"--items-imageLight": items[selectedItemIndex].imageLight,
"--items-imageDark": items[selectedItemIndex].imageDark,
'--items-imageLight': '10px', // existing user's defined variable
}}
sx={theme => ({
fontSize: 'var(--items-imageLight)',
backgroundImage: "var(--items-imageDark)",
...theme.applyStyles("light", {
backgroundImage: "var(--items-imageLight)", // ❌
})
})}
/>;
packages/mui-codemod/src/v6.0.0/sx-prop/test-cases/sx-dynamic2.actual.js
Show resolved
Hide resolved
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.
Sorry for the delay on the review @siriwatknp
'& .MuiAccordion-region': {}, | ||
'& .MuiAccordionDetails-root': {}, |
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.
Are these empty object required?
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.
Let me improve 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.
packages/mui-codemod/src/v6.0.0/sx-prop/test-cases/sx-css-vars.expected.js
Show resolved
Hide resolved
@mnajdova @DiegoAndai Could you do another review? |
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.
Looks good! Only one small nit pick
|
||
describe('@mui/codemod', () => { | ||
describe('v6.0.0', () => { | ||
describe('basic sx-v6', () => { |
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.
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 did not know it existed. To be honest, I prefer writing test separately instead of using .forEach
. The reason is debugging.
With multiple it()
, you can use .skip
or .only
to debug a test (I frequently do this all the time), but it's not possible with .forEach
. If you have a way to debug each test but still using .forEach
, I'm all ears.
Review suggestion
Please see the test cases (
*.actual
and*.expected
). Feel free to suggest more test cases.Implementation
Reuses some functions from
styled
andtheme
codemods. The logic is similar but require special treatment for some scenarios, e.g. there is novariants
insx
prop.