-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
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 https://github.com/mui/material-ui/issues/15695\#ref-commit-7fd1a29
…gnUpForm.tsx lint, cast StyledContainer to typeof Container per https://github.com/mui/material-ui/issues/15695\#ref-commit-7fd1a29
…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
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. |
oops that was a fork! |
I am able to reproduce these failures locally so I think they're legit. |
OK it looks to me like, under the mobile viewport, the test |
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 |
@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 }); |
There was a problem hiding this comment.
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
@timheilman thank you so much for doing this and sorry for the delay in getting this in! |
Closes #1418 and partial of #1508
I depended heavily on the codemods, especially jss-to-styled, because I am not a CSS expert.