[codemod] Add theme-v6 migration
#42056
Conversation
Netlify deploy previewhttps://deploy-preview-42056--material-ui.netlify.app/ Bundle size report |
There was a problem hiding this comment.
Design-wise, it looks good! I have a question, though, and it's not exactly related to the codemod:
As I see, currently, to add custom light/dark styles we should do something like this:
// light
{
props: {
color: 'secondary',
variant: 'outlined',
},
style: {
color: gray[900],
},
},
// dark
{
props: {
color: 'secondary',
variant: 'outlined',
},
style: {
...theme.applyStyles('dark', {
color: gray[300],
}),
},
},
Is it possible to have something like this instead?
// light
{
props: {
color: 'secondary',
variant: 'outlined',
},
style: {
color: gray[900],
...theme.applyStyles('dark', {
color: gray[300],
}),
},
},
IMHO it would be easier to organize and reduce some lines of code.
On a different note, we're working on the Dashboard template right now, and the idea is to have it ready for the React Conf. Should we migrate the theme there too? cc @noraleonte
|
@zanivan Yes. What you have proposed is already possible. It might be a limitation on the codemod side that the output is like that. But if you are writing the variants manually, you can do the above. |
Good point. I'd say it's good to have because matching the props and merging the styles with codemod is a lot of work (for sure it can be done). I will improve it when I have time but at least it works at the moment. |
| if (path.parent.parent.parent.get('key', 'name').value === 'styleOverrides') { | ||
| args = [path.node]; | ||
| } | ||
|
|
||
| // 1. collecting styles that should be tranformed | ||
| args.forEach((arg) => { |
There was a problem hiding this comment.
args always has one element, right? Why turn it into an array? Was the original intent to collect all nodes first?
There was a problem hiding this comment.
I didn't review this in-depth (it's a huge one, and reading large codemod code is confusing 😅), but I assume this is just extracting the previous code into a function, right?
There was a problem hiding this comment.
I didn't review this in-depth
I would not recommend either. Checking the test cases should be enough.
Since it was an issue with the codemod, I went ahead and merged the duplicated props—there weren't so many, actually. Let me know if that's ok, or if anything needs to be corrected |
| const transformed = root.toSource(printOptions); | ||
|
|
||
| if (shouldTransform) { | ||
| // recast adds extra newlines that we don't want, https://github.com/facebook/jscodeshift/issues/249 |
Most of the code comes from #41743 because the style conversion is the same.
Templates: https://deploy-preview-42056--material-ui.netlify.app/material-ui/getting-started/templates/
Docs: https://github.com/siriwatknp/material-ui/tree/codemod/theme-sx/packages/mui-codemod#theme-v6
Tested by transforming all the template themes.