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] Fix Divider props codemod #42919

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Jul 12, 2024

The divider-props codemod (introduced in #40947) removes the light prop and adds sx={{ opacity: 0.6 }} if no opacity is already passed to the sx prop.

I noticed that the codemod adds the opacity even when light={false}, which seems wrong:

// input
<Divider light={false} />;

// output
<Divider sx={{ opacity: "0.6" }} />;

The correct output should be:

<Divider />;

This PR fixes the codemod and adds some tests cases to also cover the presence of light: false in the MuiDivider.defaultProps of the theme object.


@sai6855 let me know if my reasoning is correct.

@aarongarciah aarongarciah added component: divider This is the name of the generic UI component, not the React module! package: codemod Specific to @mui/codemod labels Jul 12, 2024
@mui-bot
Copy link

mui-bot commented Jul 12, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a430288

@aarongarciah aarongarciah marked this pull request as ready for review July 12, 2024 15:05
key: 'opacity',
expression: j.literal('0.6'),
});
if (!lightProp) {
Copy link
Member Author

@aarongarciah aarongarciah Jul 12, 2024

Choose a reason for hiding this comment

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

Most of the changes above are because of the indentation change due to this early return.

Copy link
Member

Choose a reason for hiding this comment

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

image

Check hide whitespace could help the review.

const isLightPropTruthy =
!!lightProp.value?.expression.value || lightProp.value?.expression.value === undefined;

if (!isLightPropTruthy) {
Copy link
Member Author

@aarongarciah aarongarciah Jul 12, 2024

Choose a reason for hiding this comment

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

If light={false} we already removed the prop so we don't continue with adding opacity to the sx prop.

Copy link
Contributor

@sai6855 sai6855 left a comment

Choose a reason for hiding this comment

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

Test cases looks good, didn't dig deeper into codemod implementation

@aarongarciah
Copy link
Member Author

@sai6855 the implementation is almost the same, just switched to early returns and added a check to do nothing in case light={false}.

Copy link
Contributor

@sai6855 sai6855 left a comment

Choose a reason for hiding this comment

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

Yes, checked again, implementation is mostly same 👍. Reviewing codemods is always bit overwhelming 😅

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍

@aarongarciah aarongarciah removed the request for review from DiegoAndai July 17, 2024 06:16
@aarongarciah aarongarciah merged commit 7b4967a into mui:next Jul 17, 2024
19 checks passed
@aarongarciah aarongarciah deleted the fix-divider-props-codemod branch July 17, 2024 06:24
joserodolfofreitas pushed a commit to joserodolfofreitas/material-ui that referenced this pull request Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: divider This is the name of the generic UI component, not the React module! package: codemod Specific to @mui/codemod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants