-
-
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
[codemod] Add styled v6 transformation #41743
Conversation
packages/mui-codemod/src/v6.0.0/styled/test-cases/NestedSpread.expected.js
Show resolved
Hide resolved
Netlify deploy previewhttps://deploy-preview-41743--material-ui.netlify.app/ Bundle size report |
packages/mui-codemod/src/v6.0.0/styled/test-cases/BasicStyled.expected.js
Show resolved
Hide resolved
packages/mui-codemod/src/v6.0.0/styled/test-cases/BasicStyled.expected.js
Outdated
Show resolved
Hide resolved
It would be great if we can create a draft POC PR that runs this on our repo (or at least in the @mui/material) package. |
Here is the PR that runs the codemod on several components https://github.com/mui/material-ui/pull/41766/files#diff-eb5bb0f7d6fd1be741c79c303d033419ee5eefea4124632e06c5b0f4bcf5c0d4 |
cc @mnajdova the transformation looks all correct with no visual regression. |
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 think this is a great first iteration. Can we add a description in the README.md under the v6 section? I would recommend writing out examples of what is supported, then when we add more features, we can extend the descrption.
cc @DiegoAndai do we have other place where we keep the descriptions of the codemods available for v6?
Done. |
Not for the codemods specifically, but we already have the migration guides for Material and System, so whenever we add breaking changes, we should point to the corresponding codemods. |
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 left few comments to be checked before merging. The transformations look good 👍
@@ -0,0 +1,60 @@ | |||
const FormHelperTextRoot = styled('p')(({ theme, ownerState, disabled }) => ({ |
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.
Maybe in the future we can add support for aliased imports, e.g.
import { styled as muiStyled } from '@mui/material/styles';
variant: 'normal' | ||
}, | ||
style: { | ||
backgroundColor: (theme.vars || theme).palette[ownerState.color].light |
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.
can we add comment above this line?
medium: theme.typography.pxToRem(24), | ||
large: theme.typography.pxToRem(36), | ||
}[ownerState.fontSize], | ||
// TODO v5 deprecate, v6 remove for sx |
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.
This got lost somewhere :)
…d/pigment-styled
…d/pigment-styled
This codemod is benefit for Material UI and any project that are using
styled
v5. The transformation will remove the use of props in the style argument and replace them withvariants
.Review suggestion
Please focus on the test cases (not the codemod implementation). Feel free to suggest more test cases that we should cover.
Target components
See the result.
Out-of-scope transformations
❌ Dynamic values, for example:
Since Emotion does not support callback as a value, developers have to manually fix this by using inline style + CSS variables
❌ dynamic reference from the theme, e.g. color palette: