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

[codemod] Add a codemod for the Box sx prop #23465

Merged
merged 6 commits into from
Nov 12, 2020
Merged

Conversation

mbrookes
Copy link
Member

box-sx-prop

Updates the Box API from separate system props to sx.

The diff should look like this:

-<Box border="1px dashed grey" p={[2, 3, 4]} m={2}>
+<Box sx={{ border: "1px dashed grey", p: [2, 3, 4], m: 2 }}>
find src -name '*.js' -print | xargs npx jscodeshift -t node_modules/@material-ui/codemod/lib/v5.0.0/box-sx-prop.js

@mbrookes mbrookes added this to the v5 milestone Nov 10, 2020
@mbrookes mbrookes requested a review from mnajdova November 10, 2020 11:21
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 10, 2020

Details of bundle changes

Generated by 🚫 dangerJS against e3c1e13

<Box border="1px dashed grey" p={[2, 3, 4]}>
<Box component="span" clone p={{ xs: 2, sm: 3, md: 4 }} m={2} border="1px dashed grey">
<Button component="span">Save</Button>
</Box>
Copy link
Member

Choose a reason for hiding this comment

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

As we are going to merge #23454 soon and add deprecation in v4, should we add two more scenarios:

- <Box border="1px dashed grey" p={[2, 3, 4]} css={{ m: 1 }}>
+ <Box sx={{ border: "1px dashed grey", p: [2, 3, 4], m: 1 }}>

- <Box border="1px dashed grey" p={[2, 3, 4]} sx={{ m: 1 }}>
+ <Box sx={{ border: "1px dashed grey", p: [2, 3, 4], m: 1 }}>

Copy link
Member Author

Choose a reason for hiding this comment

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

For the first scenario, I didn't want to add that until #23454 is merged. For the second, the sx prop doesn't exist in v4.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, let’s do that after we have the breaking change and deprecation PRs in.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, #23454 is merged. I'd like to address it as a follow-up PR though. This one was hard enough! 😅

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I will work tomorrow on deprecation for v4 and we can update it after that one is merged too :)

@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Nov 10, 2020
@mbrookes mbrookes merged commit 306dc35 into mui:next Nov 12, 2020
@mbrookes mbrookes deleted the box-sx-codemod branch November 12, 2020 13:47
@oliviertassinari
Copy link
Member

Should we run the codemod on the next branch? 🙌

@mbrookes
Copy link
Member Author

We could, but I believe @eps1lon has covered it here: #23285

@ghost
Copy link

ghost commented Jan 19, 2021

When running find src -name '*.js' -print | xargs npx jscodeshift -t node_modules/@material-ui/codemod/v5.0.0/box-sx-prop.js I receive 'does not exist', what should I install to run it? Material UI alpha version 23

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 19, 2021

@sphinxs We are taking one step back: #24485.

@ghost
Copy link

ghost commented Jan 19, 2021

I see, thanks.

@mbrookes
Copy link
Member Author

We are going backward

Not quite backward, since it supports both APIs in parallel. I guess we need to decide which API to use in the examples.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 19, 2021

I guess we need to decide which API to use in the examples

@mbrookes I think that we could use the prop anytime it's possible. It's simpler and hopefully enough for +80% of the cases cc @mnajdova

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants