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

Build/upgrade mui v4 to v5 #1522

Merged

Conversation

timheilman
Copy link
Contributor

@timheilman timheilman commented Mar 6, 2024

Closes #1418 and partial of #1508

I depended heavily on the codemods, especially jss-to-styled, because I am not a CSS expert.

First, the color changes indicated at https://mui.com/material-ui/migration/v5-component-changes/#update-default-indicatorcolor-and-textcolor-prop-values

Second, remove the the trailing "px" a number rather than string when passing values to the react-virtualized List component.  See
https://mui.com/material-ui/migration/v5-style-changes/\#%E2%9C%85-remove-px-suffix
this was causing the slider-based cypress tests to fail.
appBar was still over the date picker in mobile screen size, failing one of the cypress:run:mobile tests.  See also
https://mui.com/material-ui/migration/v5-component-changes/\#fix-z-index-issues
@cypress-app-bot
Copy link

…inLayout.tsx

review, remove class for root and use ampersand instead, adding a flexGrow:1 to get styling as before
…vBar.tsx,

review, appBarShift was incorrectly considered a child of root; fixed that with removal of space between & and ., lint
…tificationListItem.tsx

review, lint, no issues
…gnInForm.tsx

lint, cast StyledContainer to typeof Container per mui/material-ui#15695
…gnUpForm.tsx

lint, cast StyledContainer to typeof Container per mui/material-ui#15695
…ansactionAmount.tsx

lint, remove space between & and . because classes are conditionally applied
…ansactionCreateStepOne.tsx

lint and review, no issues
…ansactionCreateStepTwo.tsx

lint, review, no issues
…ansactionCreateStepThree.tsx

lint, review, no issues
@timheilman
Copy link
Contributor Author

OK, I've made some small progress on the TransactionDetail unintended layout change.
The offending commit is 552005b, resulting from the command:

npx @mui/codemod@latest v5.0.0/preset-safe src

@timheilman
Copy link
Contributor Author

@timheilman
Copy link
Contributor Author

Looks like the removal of alignItems and justifyContent props from the Grid component is the problem.

Actually, not. My reading of that is that the props are still OK; just that MUI system took them over from the component. And CSS inspection shows them applied.

For the avatar group root class, two CSS properties differ: a flex-direction: "row-reverse" is coming in, and font-size: is 16px rather than 14px.

@timheilman timheilman marked this pull request as draft June 10, 2024 00:41
@timheilman
Copy link
Contributor Author

the flex-direction issue looks like it was from a change in the <AvatarGroups> component, and a fix is pushed. The font-size issue, and other css issues with text input elements seem to be related to changes in defaults from the jss styling engine to the emotion styling engine. Next I'll look for how to set emotion defaults to the previous jss defaults.

@AtofStryker
Copy link
Contributor

I reran the percy jobs and the diff looks pretty close to me. I would say this is acceptable. @timheilman did the default theme colors change slightly?

It also looks like there is a failure in the mobile viewport tests? Though I don't see the error on my updated mirror PR #1550 so I am wondering if it is legit

@timheilman
Copy link
Contributor Author

I'm back into my work week and have to table this in favor of paid work for now. I'm actually cutting my teeth here -- I've never used MUI v4 nor v5 before, nor any theming engine for that matter. It does look like the palette changed, again probably because of the jss->emotion switch, but I would need to debug with the css developer tools to find out why. Of course the test failures need looking into.

If anyone else wants to take over the branch/PR, that's fine by me. Or I will look at it again when I get the time.

@timheilman
Copy link
Contributor Author

See replayio-public#113

@timheilman timheilman closed this Jun 29, 2024
@timheilman
Copy link
Contributor Author

oops that was a fork!

@timheilman timheilman reopened this Jun 29, 2024
@timheilman
Copy link
Contributor Author

It also looks like there is a failure in the mobile viewport tests?

I am able to reproduce these failures locally so I think they're legit.

@timheilman
Copy link
Contributor Author

OK it looks to me like, under the mobile viewport, the test ui/transaction-view.spec.ts is failing due to issue 29776.

@timheilman timheilman marked this pull request as ready for review June 30, 2024 14:55
@timheilman
Copy link
Contributor Author

I'll check back on this and if the font sizes and theming colors remain blockers to merging investigate how to fix them then. I'm not happy about the { force: true } fix to the failing mobile viewport tests, but as far as I can tell those test failures are exhibiting a genuine bug in cypress.

@AtofStryker
Copy link
Contributor

@jennifer-shehane the changes in percy look acceptable to me

@@ -58,7 +60,8 @@ describe("Transaction View", function () {
});

it("comments on a transaction", function () {
cy.getBySelLike("transaction-item").first().click();
// { force: true } is a workaround for https://github.com/cypress-io/cypress/issues/29776
cy.getBySelLike("transaction-item").first().click({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems ok until we can address the issue

@jennifer-shehane jennifer-shehane merged commit 31b7fa5 into cypress-io:develop Aug 27, 2024
35 of 36 checks passed
@AtofStryker
Copy link
Contributor

@timheilman thank you so much for doing this and sorry for the delay in getting this in!

@MikeMcC399 MikeMcC399 mentioned this pull request Aug 27, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to MUI v5
5 participants