vanvalenlab / kiosk-frontend

DeepCell web application built using NodeJS, Express, React, and Webpack.
Other
1 stars 0 forks source link

Upgrade to MUI v5 #181

Closed tddough98 closed 2 years ago

tddough98 commented 2 years ago

Similar to https://github.com/vanvalenlab/deepcell-label/pull/304 , this updates to MUI v5. The commits initially follow the migration guide exactly, including

I did most of the styling migration manually. I tried using their styling codemod, but the results had some layout issues, did not take advantage of new shortcuts like m for margin, and had a significant amount of boilerplate.

As we mostly use one-off styling, I used the new sx prop throughout for styling. I factored out a few shared styles into a const when the class was repeated 3+ times, like the paperStyle in Faq.js.

Running the page locally let me verify that everything looked good for the homepage, the About page, and the FAQ. The predict page calls an API to get the job types, so I couldn't verify everything on that page. Other than some minor sizing and color changes, everything seemed the same.

I was running into an error when running the frontend locally Failed to load parser 'babel-eslint' declared in '.eslintrc.yml': Cannot find module 'babel-eslint', so I tried adding babel-eslint to devDependencies, which worked until I made more edits and had to reload the frontend. This is a linter, so definitely no issues for production, but if anyone who's been working on the frontend has seen or fixed this with local config, let me know how to fix it.

I also updated the testing matrix as updating react-scripts dropped support for node v12.

tddough98 commented 2 years ago

The react-scripts bump was causing test suite issues 😅 reverted that. While troubleshooting, I changed tests with async done to just async (), which I believe is more correct https://jestjs.io/docs/asynchronous and should prevent future issues.

I think the 0.2% drop in coverage is not meaningful because this PR is mostly styling changes, not functionality

msschwartz21 commented 2 years ago

Just to be safe, I am going to deploy and test the frontend in a live deployment before approving, but seems like it should be fine.