cypress-realworld-app
cypress-realworld-app copied to clipboard
cypress-io
Build/upgrade mui v4 to v5
Closes #1418 and partial of #1508
I depended heavily on the codemods, especially jss-to-styled, because I am not a CSS expert.
- Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
@MikeMcC399 OK merge conflicts are resolved.
Also, in the meantime I discovered https://github.com/cypress-io/cypress-realworld-app/issues/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 https://github.com/cypress-io/cypress-realworld-app/pull/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. The offending commit is 552005ba757d366545fb772e9e8c8f524ae99445, resulting from the command:
npx @mui/codemod@latest v5.0.0/preset-safe src
Looks like the removal of alignItems and justifyContent props from the Grid component is the problem.
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.
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.
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 https://github.com/cypress-io/cypress-realworld-app/pull/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.
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.
OK it looks to me like, under the mobile viewport, the test ui/transaction-view.spec.ts is failing due to issue 29776.
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.
@timheilman thank you so much for doing this and sorry for the delay in getting this in!