Build/upgrade mui v4 to v5 #1522
Conversation
…add @mui/icons-material
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
|
followed by lint fixes and manual review
…nkAccountForm.tsx linting and review
…mmentForm.tsx review, lint
…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
…vDrawer.tsx review, lint, no issues
…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
…eletonList.tsx lint, review, no issues
…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
There was a problem hiding this comment.
It's great you've picked up this migration. Well done! 👍🏻
The data/database.json should not be part of the PR. There is something unclean in the repo causing the file to get changed, when it ought to be static. I have noticed this before after running tests but I didn't investigate it further. 🙁
|
mui v5, o how it needs a hero... |
…lock merge resolution
which is not failing after the upgrade to MUI v5. See cypress-io#1278
|
@MikeMcC399 OK merge conflicts are resolved. Also, in the meantime I discovered #1278 . Indeed this test is passing in firefox after this upgrade to MUI v5. This upgrade was a lot of work. A lot of slow, "grunt" work -- updating a page, verifying it still rendered correctly manually, verifying it still passed tests. The focus was: eliminate the build warnings in #1418 . The build warning is eliminated. Whatever further changes may need to be made, this PR is a step in the right direction, since MUI v4 is dying. |
|
really nice work on this @timheilman ! I reviewed locally and things look as expected on my end. I opened #1550 so we can run percy to see visual regressions. I will close it of course in favor of your PR. The only purpose it serves is to run a few additional jobs we don't normally run for external contributors. |
|
@timheilman The percy snapshots in the forked PR seem to be highlighting some changes in styles on the Transaction View page which don't look desirable. Could you take a look?
|
|
I'm unfamiliar with percy but I will try to understand what's going on and see what I can do when I get some free cycles. |
|
@timheilman Percy is just a visual regression testing framework. So it will take screenshots before a commit and screenshots after a commit and compare them for the entire app where you tell it to take screenshots. Unfutunately contributors don't have access to our Percy account so can't directly review the screenshots. The screenshots after this change indicate that the 'Transaction View page' looks different after this change in this PR, which I don't think is intentional. You can look at the change within Test Replay to narrow down the cause. You can see the avatars are in a different place in those. |
|
OK, I've made some small progress on the TransactionDetail unintended layout change.
|
|
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 |
AvatarGroup in mui5.
no longer overlapping. Instead eat up space to right of avatars.
|
the flex-direction issue looks like it was from a change in the |
modifying input styles
|
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 |
|
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. |
Closes #1418 and partial of #1508
I depended heavily on the codemods, especially jss-to-styled, because I am not a CSS expert.