MUI v9 upgrade (material-renderers) - #2614
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Hi @jgteske , thank you very much for this contribution ❤️
The changes already look pretty good to me and worked well using the example app!
Please have a look at the two inline comments.
Besides that, please add a note to the MIGRATION.md regarding the MUI update. See https://github.com/eclipsesource/jsonforms/blob/master/MIGRATION.md#update-to-material-ui-v5-in-react-material for a previous Material UI update note there.
Note: I opened a follow up issue #2615 that was discovered here but the same behavior was already present before the upgrade in this PR. Thus, we do not need to fix this here. Even if you want to contribute this (which of course is always welcome ❤️ ), I think a separate PR is preferable so we can merge this PR ASAP and create a pre-release.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Upgrades @jsonforms/material-renderers from MUI 7 to MUI 9 (plus x-date-pickers 8 to 9) and migrates the props that v9 dropped: inputProps/InputLabelProps become slotProps.htmlInput/slotProps.inputLabel, Grid's justifyContent/alignItems move into sx, Stack width becomes sx, and one icon is renamed. The lockfile diff is clean (only MUI plus a transitive @babel/runtime bump) and there are no unrelated drive-by changes.
Points worth attention:
MuiAutocompletestill carries the v7 compatibility shim, which contradicts the dropped v7 peer support (lucas-koehler's open thread). Beyond removing it, the hand-rolled three-slot merge there is the most fragile piece of the PR.directionis handled two different ways within the same PR: moved intosxinutil/layout.tsxbut kept as a Grid prop inMaterialTableControl/ExpandPanelRenderer.- The
inputPropsusages on the InputBase based cells were not touched, so it is unclear whether they were checked against v9. - Still open from the previous review: the
ErrorOutlineOutlinedicon and the MIGRATION.md note.
I could not verify anything in the UI (no network access in this run, so neither the deploy preview nor a local build was reachable). Since the sx migration changes how Grid alignment is applied, a maintainer should eyeball the array table rows, accordion headers and horizontal layouts.
Thanks for your code review and your requested changes. |
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up on the two commits pushed since my last review (f9e2e43b, 87a79fdd).
Both fixes landed cleanly:
MuiAutocomplete.tsx: the v7 compatibility shim is gone andrenderInputnow spreadsparams.slotPropsdirectly, only overridinginputLabelfor theshrinkbehavior. This addresses both my slot-merging concern and lucas-koehler's request to drop v7 support.ValidationIcon.tsx: icon renamed toErrorOutlineOutlinedas requested.
The direction question on layout.tsx is resolved too: the author's explanation checks out (dynamic 'row'|'column' there vs. hardcoded 'row' literals elsewhere), not an inconsistency, so I resolved that thread.
Nothing new to flag in the diff since last time. Still outstanding from the earlier round: the MIGRATION.md note lucas-koehler asked for hasn't been added yet, a maintainer should confirm that's still expected before merge.
There was a problem hiding this comment.
Hi @jgteske , thanks for the updates! It almost LGTM. Please have a look at the one remaining inline comment :)
Please also add the migration entry to MIGRATION.md
lucas-koehler
left a comment
There was a problem hiding this comment.
@jgteske The changes look good to me now 🚀 ! Thanks for the contribution and all the updates ❤️
I added a small additional note to the MIGRATION.md.
No description provided.