Skip to content
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

MUI V5 Migration #554

Merged
merged 26 commits into from
Nov 17, 2023
Merged

MUI V5 Migration #554

merged 26 commits into from
Nov 17, 2023

Conversation

sboleyn
Copy link
Contributor

@sboleyn sboleyn commented Nov 3, 2023

The majority of changes are relating to the MUI 5 migration and cover Breaking changes parts 1 and 2 of the migration documentation. A fix to a few random console errors were also wrapped into these changes.

@sboleyn sboleyn requested a review from psarando November 3, 2023 21:42
Copy link

socket-security bot commented Nov 3, 2023

Copy link
Member

@psarando psarando left a comment

Choose a reason for hiding this comment

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

Overall LGTM 👍 🎉 ✨

Thanks for tackling this migration. It looks like it turned out to be much more tedious than I anticipated 😩 Did you use any of their codemods for some of these migrations?

I had a couple of minor suggestions, and I did catch at least 1 bug, but otherwise :shipit: when ready! 🎊

package.json Outdated Show resolved Hide resolved
src/__tests__/data.js Outdated Show resolved Hide resolved
src/components/analyses/listing/TableView.js Show resolved Hide resolved
src/components/tools/details/Drawer.js Outdated Show resolved Hide resolved
src/components/vice/loading/styles.js Outdated Show resolved Hide resolved
src/components/analyses/landing/DotMenuItems.js Outdated Show resolved Hide resolved
src/components/layout/AppBar.js Outdated Show resolved Hide resolved
src/components/vice/admin/filter/sections.js Outdated Show resolved Hide resolved
@psarando
Copy link
Member

psarando commented Nov 9, 2023

By the way, I noticed there were no chromatic tests for this PR. Maybe your GitHub action was disabled? Maybe see if you can turn it back on at https://github.com/sboleyn/cyverse-de-sonora/actions before pushing more commits?

edit: Actually I just checked the logs for your chromatic actions (TIL), and there seems to be an issue with the package.json and package-lock.json files. Maybe if you npm install --save "@mui/lab@^5" and commit those package.json and package-lock.json changes, it will fix the chromatic build error.

src/components/tools/details/Drawer.js Outdated Show resolved Hide resolved
src/components/apps/details/Drawer.js Show resolved Hide resolved
src/components/analyses/details/Drawer.js Show resolved Hide resolved
src/components/data/styles.js Show resolved Hide resolved
src/components/data/styles.js Outdated Show resolved Hide resolved
src/components/search/detailed/styles.js Outdated Show resolved Hide resolved
src/components/tools/details/Drawer.js Show resolved Hide resolved
src/components/vice/loading/styles.js Show resolved Hide resolved
Copy link
Member

@psarando psarando left a comment

Choose a reason for hiding this comment

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

Sorry for the thrash in this PR, but after reviewing the latest Chromatic tests, and then testing all of these details drawers at different sizes with these latest changes, I now think my assumption about the original intention for these drawer sizes was incorrect.

Some of the breakpoints changes reverted in the last commit look fine, but I found some details drawers that we probably should revert again, and fix the up breakpoint size instead 😖

I'll leave it up to you if you wish to fix those breakpoints I commented on in this PR, or we can merge this PR as-is and fix them in a follow-up PR.

src/components/analyses/details/Drawer.js Outdated Show resolved Hide resolved
src/components/apps/details/Drawer.js Outdated Show resolved Hide resolved
src/components/data/styles.js Outdated Show resolved Hide resolved
src/components/subscriptions/details/Drawer.js Outdated Show resolved Hide resolved
src/components/tools/details/Drawer.js Outdated Show resolved Hide resolved
src/components/vice/loading/styles.js Outdated Show resolved Hide resolved
Copy link
Member

@psarando psarando left a comment

Choose a reason for hiding this comment

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

LGTM :shipit: 👍 🎉

@sboleyn sboleyn merged commit 14aa302 into cyverse-de:master Nov 17, 2023
4 of 6 checks passed
@psarando psarando mentioned this pull request Nov 22, 2023
psarando added a commit to psarando/cyverse-de-sonora that referenced this pull request Feb 29, 2024
Probably broken by MUI v5 Autocomplete upgrade in cyverse-de#554
psarando added a commit to psarando/cyverse-de-sonora that referenced this pull request Feb 29, 2024
Probably broken by MUI v5 Autocomplete upgrade in cyverse-de#554
psarando added a commit to psarando/cyverse-de-sonora that referenced this pull request Mar 4, 2024
Probably broken by MUI v5 Autocomplete upgrade in cyverse-de#554
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