Skip to content

Update to MUI 7 #4139

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Update to MUI 7 #4139

wants to merge 3 commits into from

Conversation

marlo-longley
Copy link
Member

@marlo-longley marlo-longley commented Apr 11, 2025

This was mostly created using MUI's codemods.
The Grid2 component has moved back into the Grid namespace. That is the biggest change.
(for testing purposes, also included is @lutzhelm's syntax for externalizing peer dependencies but I can slice this out into another PR.)

Visually, the changes seem to have no impact, but of course more eyes are better!

I am interested in this upgrade because of the solution for better ESM support: mui/material-ui#45495. This will fix some issues reported during plugin integration such as #4136

If this is approved and merged and we cut a new alpha release, I have PRs drafted locally for several plugins to support this change. And some like image-tools seem to require zero changes to support 7.

@marlo-longley marlo-longley marked this pull request as ready for review April 11, 2025 16:16
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.91%. Comparing base (48ca7e0) to head (07789cd).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4139      +/-   ##
==========================================
- Coverage   94.91%   94.91%   -0.01%     
==========================================
  Files         324      324              
  Lines       16147    16144       -3     
  Branches     2533     2532       -1     
==========================================
- Hits        15326    15323       -3     
  Misses        816      816              
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@lutzhelm lutzhelm left a comment

Choose a reason for hiding this comment

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

Not an entire review, just wanted to leave this here for them moment.

(Thanks for figuring out a solution.)

@@ -20,7 +20,7 @@ export function SelectCollection({
return (
<Grid container justifyContent="center" alignItems="center">
<Grid container direction="column" alignItems="center">
<Typography variant="h4" paragraph>
<Typography variant="h4" component="p">
Copy link
Contributor

Choose a reason for hiding this comment

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

The show collection button is no longer aligned in the center.
select_collection

Copy link
Contributor

@lutzhelm lutzhelm Apr 23, 2025

Choose a reason for hiding this comment

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

The cause for that is not in the Typography change, of course. In the old version, the div for the row Grid (the component that is now GridLegacy) two lines above had a CSS width of 100% that is missing in the new version. Adding size="grow" to the row Grid would be one option to solve this.

Copy link
Member Author

@marlo-longley marlo-longley Apr 23, 2025

Choose a reason for hiding this comment

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

@lutzhelm thank you for finding this. But I am not seeing it? This looks centered to me.

Screenshot 2025-04-23 at 5 22 58 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

But I do see this!

Screenshot 2025-04-23 at 5 26 38 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked out 9ada8ca just to double check, and this is what it looks like to me - centered as it should:

welcome-to-mirador

I'm a bit confused and wonder where those differences come from. I tested with Firefox 136, Chromium 136 and Chrome 135 on Ubuntu Linux, running Mirador with NodeJS 20.18.1 and npm 10.8.2.

Copy link
Contributor

@lutzhelm lutzhelm left a comment

Choose a reason for hiding this comment

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

Otherwise, I found no issues.

I was also able to use this in the share, download and image-tools plugins and had no trouble with tests or builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

While the manifest list itself looks way better than before, ManifestListItemError looks a bit disordered now.

ManifestListItemError

Copy link
Member Author

@marlo-longley marlo-longley Apr 23, 2025

Choose a reason for hiding this comment

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

Thanks for finding this. I actually started seeing even worse behavior where the stacking was overlapping. Which is odd that neither of us saw that before so I'm curious if you can reproduce prior to 07789cd . I added a separate commit 07789cd Rework the manifest list layout to this PR to address that -- please let me know what you see, but here's what I see now:

Screenshot 2025-04-23 at 6 13 03 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I see on commit 07789cd (your latest from yesterday):

manifest-list-07789cd

These differences don't really inspire confidence. But if there aren't any functional problems, we could fix other surfacing related issues along the way.

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.

2 participants