DT-1110: Migrate away from @mui/styles Part 1 #1730

snf2ye · 2025-01-03T15:53:49Z

Background

We must discontinue use of @mui/styles in order to migrate from React 17 to 18. (source)

@mui/styles was deprecated with the release of MUI Core v5 in late 2021. It is not compatible with React.StrictMode or React 18+, and it will not be updated. Please use @mui/system instead.

There is guide for this migration here: https://mui.com/material-ui/migration/migrating-from-jss

Options considered:

Primarily use the sx prop and use styled components for styles that are used multiple times - From migration guide - "We recommend sx API over styled for creating responsive styles or overriding minor CSS"
tss-react - Easiest change, but still would require using copilot per component. It would be more of a lateral step.

This PR uses the first option and mostly sticks the sx prop. We could also use styled components here where it makes sense. I see this first option as an upgrade that should help us align with best practices. Given that we have to touch every file manually and I think we can utilize copilot for the bulk of the work, I would prefer to do the harder change rather than the lateral change.

Steps for Upgrades

Pre-checks:

Review the styles listed at the beginning of the file. Are they all referenced in the component? (Apparently if there are styles that don't match up to a component, it will try to infer where the component with those styles should go and add it)

Run copilot command

Can you migrate this component away from using JSS and references to '@mui/styles' and instead use '@mui/system'? Please use the "sx" prop wherever possible instead of "styled". If you need to use the "sx" prop, then we must use the material UI version of the component (e.g. Box instead of div and add an import statement at the beginning of the file). We must also switch any css references of direct numbers to include px (e.g. '10px' instead of 10)

Apply prettier
Review UI and code generated
Compare UI/styling between local UI and dev

Common Issues

It doesn't handle theme changes well - 30ca7c2

What step is the copilot upgrade taking?

Remove the import statements for withStyles, createStyles, WithStyles and any other references from @mui/styles.
Add import statements for Box from @mui/material. We need to use Box instead of div and span so that we can use the sx prop. This prop is only available on components from Material UI. If we want to still use generic component, we can instead use styled components.
Update the component to use the sx prop for styling. We move any styles listed for a given class from the style constant inline into an sx prop.

Example
Before with @mui/styles

const styles = (theme: CustomTheme) => ({
  wrapper: {
    display: 'flex',
    justifyContent: 'center',
    marginTop: '1em',
  }
);
....
 <div className={classes.wrapper}>
...
</div>

With sx prop

 <Box sx={{ display: 'flex', justifyContent: 'center', marginTop: '1em' }}>
...
</Box>

Components Impacted in this PR

Recording Video

HomeView and 2) DatasetView

JobView

SnapshotView

DatasetOverviewn and 6) DatasetOverviewPanel

DatasetAccess and 8) UserList

SnapshotAccessRequestTable

cypress · 2025-01-03T15:57:45Z

jade-data-repo-ui Run #3920

Run Properties: Passed #3920 • 10fabcb73b ℹ️: Merge 3a12537f4016d5606888b62d92fcf72522e8801c into 79c0423d9cfcafed88655352e6c8...

Project	`jade-data-repo-ui`
Branch Review	`sh/dt-1110-remove-mui-styles-part1`
Run status	`Passed #3920`
Run duration	`02m 52s`
Commit	`10fabcb73b ℹ️: Merge 3a12537f4016d5606888b62d92fcf72522e8801c into 79c0423d9cfcafed88655352e6c8...`
Committer	`Shelby Holden`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`17`
View all changes introduced in this branch ↗︎

src/components/JobView.tsx

snf2ye · 2025-01-06T16:21:14Z

src/components/dataset/overview/DatasetOverviewPanel.jsx

+        sx={{
+          top: '112px',
+          bottom: '0px',
+          flexShrink: 0,
+          height: 'initial',
+          zIndex: 10,
+          minWidth: 300,
+          width: '40%',
+          position: 'absolute',
+          transition: () =>
+            theme.transitions.create('width', {
+              easing: theme.transitions.easing.sharp,
+              duration: theme.transitions.duration.enteringScreen,
+            }),
+          '&.drawerClose': {
+            transition: () =>
+              theme.transitions.create('width', {
+                easing: theme.transitions.easing.sharp,
+                duration: theme.transitions.duration.leavingScreen,
+              }),
+            overflowX: 'hidden',
+            width: '0px',
+            minWidth: '0px',
+            [theme.breakpoints.up('sm')]: {
+              width: '0px',
+            },


For more complicated styles like this, it may make sense to pull it into a styled component to help with code readability. But, this styling is still only used in one spot so I'm a torn over which is better (leave as sx prop vs. pull into styled component)

Since the refactoring is being done in stages, we can always pull this out into a separate style in the future.

fboulnois

One main question, otherwise lgtm:

fboulnois · 2025-01-08T14:13:30Z

src/components/UserList.tsx

+          fontSize: '14px',
+          lineHeight: '22px',
+          fontWeight: '600',
+          color: 'primary.main',


Do these need to import theme from 'modules/theme'; and use theme.palette.primary.main instead like in src/components/DatasetView.tsx? If so, a number of components in this PR have this issue with color.

Great question - I believe it is correct as is - Here's the documentation and the basic example has something very similar. We have "ThemeProvider" set up in the index, so the theme should be available to all components in the repo.

I'm possibly not referencing the theme correctly in DatasetView, but I wasn't quite sure how to do the expansion of the custom mixin.

src/components/UserList.tsx

Command used: Can you migrate this component away from using JSS and references to '@mui/styles' and instead use '@mui/system'? Please use the "sx" prop wherever possible instead of "styled". If you need to use the "sx" prop, then we must use the material UI version of the component (e.g. Box instead of div and add an import statement at the beginning of the file). We must also switch any css references of direct numbers to include px (e.g. '10px' instead of 10)

Im not sure where it got that maxwidth from Update references

- title isn't used anywhere add comma

Command: Can you migrate this component away from using JSS and references to '@mui/styles' and instead use '@mui/system'? Please use the "sx" prop wherever possible instead of "styled". If you need to use the "sx" prop, then we must use the material UI version of the component (e.g. Box instead of div and add an import statement at the beginning of the file). We must also switch any css references of direct numbers to include px (e.g. '10px' instead of 10)

- Switch again to use theme mixins - userEmail is not used

s-rubenstein · 2025-01-08T15:24:03Z

src/components/DatasetView.tsx

-  },
-);
+  return (
+    <Box sx={{ display: 'flex', justifyContent: 'center', marginTop: '1em' }}>


If you want to reduce the diff here, and keep the object abstractions, this could be <Box sx={styles(theme).wrapper}> just as easily, and that way we keep our object abstractions while moving to the sx property?

To do this you would need to have styles be a raw object rather than a method (or call it once if we have a theme we could pass), but that should be okay

That's a really interesting idea, and I'm torn about it.
I think I'm less interested in making the diff smaller (copilot is doing the heavy lifting here to move things around), and want to think about how we want this to look long term. We often use existing code as a model for new code, so what do we want our best practices to be going forward?
I think I generally like the sx prop styles to be inline for a small set of changes, but we may want to consider using styled components more frequently to keep a pseudo object abstraction. I could actually build that into the copilot request - like "if there are more than 3 css styles, use a styled component instead of the sx prop".

That's my ultimate preference as well. 3 css styles is fine for inline, more gets bulky. I also think if we have a often reused width or something - with lots of constants reused, that I'd abstract.

Turns out that its tricky to get copilot to handle these two cases 🫠 It's not great at counting, apparently (It would catch some components with >3 styles, but not all).
We may have to do this in two steps (convert to sx prop or styled and then move the rest). I think I'm also having better luck with a newer gpt model in vs code (as opposed to intellij).

rushtong

👍🏽

…stick out on the page

I think we need to use @mui/material/styles instead? This is what is referenced in docs: https://mui.com/material-ui/customization/theming/ remove @mui/styles references from theme

sonarqubecloud · 2025-01-08T17:23:42Z

Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

rjohanek · 2025-01-08T18:35:42Z

src/components/DatasetView.tsx

 import DatasetTable from './table/DatasetTable';

 import { DatasetRoles } from '../constants';

-const styles = (theme: CustomTheme) => ({
-  wrapper: {


so just a general comment, this is moving us away from having separate style objects and towards inline styling. Is that the principle that we want to follow? I thought that separate objects were preferred because they are reusable and easier to maintain. But maybe that isn't the case. What are the guidelines from the UI working group on this?

My goal in this PR is to use best practices for Material UI. We're moving from an old Material UI concept (@mui/styles), to a new one - sx prop + styled components. I don't think Terra UI uses Material UI, but I'm glad to still run this by them.

RE: separate objects that are reusable/easier to maintain: We should use styled components for this case. The "sx prop" should be used for styles that are used in one spot, styled components are reusable and then we use the global theme to share styling variables. I think this is a helpful guide here: https://mui.com/material-ui/customization/how-to-customize

snf2ye force-pushed the sh/dt-1110-remove-mui-styles-part1 branch from e71a9d8 to c83ff6d Compare January 3, 2025 16:24

snf2ye commented Jan 4, 2025

View reviewed changes

src/components/JobView.tsx Outdated Show resolved Hide resolved

snf2ye commented Jan 6, 2025

View reviewed changes

snf2ye mentioned this pull request Jan 7, 2025

DT-1125: Migrate away from @mui/styles Part 2 - ThemeProvider to use @mui/system #1731

Merged

snf2ye force-pushed the sh/dt-1110-remove-mui-styles-part1 branch 2 times, most recently from b1577c1 to 5ac0898 Compare January 7, 2025 21:34

snf2ye marked this pull request as ready for review January 8, 2025 13:55

snf2ye requested a review from a team as a code owner January 8, 2025 13:55

snf2ye requested review from rushtong and s-rubenstein and removed request for a team January 8, 2025 13:55

fboulnois approved these changes Jan 8, 2025

View reviewed changes

snf2ye added 13 commits January 8, 2025 09:59

HomeView - converted with copilot

6de264f

DatasetView - Copilot decided to go rouge and just add a title??

3cdaf30

DatasetView: Copilot dropped the mixins css

3eb2c0c

Im not sure where it got that maxwidth from Update references

SnapshotView: Prep for copilot changes

aae9af3

- title isn't used anywhere add comma

SnapshotView: copilot dropped the mixins css

e2b0a74

JobView - prep for copilot - remove unused styles

073eeae

JobView - upgrade by copilot

0d31740

JobView: cleanup

bee3a39

- Switch again to use theme mixins - userEmail is not used

SnapshotAccessRequestTable

0997ed6

DatasetOverview: Remove unused styles

849067c

DatasetOverview - Copilot update

b48ab67

s-rubenstein reviewed Jan 8, 2025

View reviewed changes

rushtong approved these changes Jan 8, 2025

View reviewed changes

snf2ye added 2 commits January 8, 2025 12:09

DatasetOverviewPanel - update with copilot

e31f67f

DatasetOverviewPanel.jsx - set the Drawer to temporary so it doesn't …

a03e3bc

…stick out on the page

snf2ye added 6 commits January 8, 2025 12:09

Index

16f1f84

I think we need to use @mui/material/styles instead? This is what is referenced in docs: https://mui.com/material-ui/customization/theming/ remove @mui/styles references from theme

DatasetOverviewPanel.jsx - clean up references

094e18d

DatasetAccess - remove styles, not referenced in component

b97a324

UserList - remove unused styles

10b60e2

UserList - update by copilot

3c645ae

JobView - switch back to more specific type

3a12537

snf2ye force-pushed the sh/dt-1110-remove-mui-styles-part1 branch from 80598fd to 3a12537 Compare January 8, 2025 17:23

rjohanek reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT-1110: Migrate away from @mui/styles Part 1 #1730

DT-1110: Migrate away from @mui/styles Part 1 #1730

snf2ye commented Jan 3, 2025 •

edited

Loading

cypress bot commented Jan 3, 2025 •

edited

Loading

snf2ye Jan 6, 2025 •

edited

Loading

fboulnois Jan 8, 2025

fboulnois left a comment

fboulnois Jan 8, 2025

snf2ye Jan 8, 2025 •

edited

Loading

s-rubenstein Jan 8, 2025 •

edited

Loading

snf2ye Jan 8, 2025

s-rubenstein Jan 8, 2025

snf2ye Jan 8, 2025

rushtong left a comment

sonarqubecloud bot commented Jan 8, 2025

rjohanek Jan 8, 2025

snf2ye Jan 8, 2025

DT-1110: Migrate away from @mui/styles Part 1 #1730

Are you sure you want to change the base?

DT-1110: Migrate away from @mui/styles Part 1 #1730

Conversation

snf2ye commented Jan 3, 2025 • edited Loading

Background

Steps for Upgrades

Common Issues

What step is the copilot upgrade taking?

Components Impacted in this PR

cypress bot commented Jan 3, 2025 • edited Loading

snf2ye Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fboulnois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snf2ye Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

s-rubenstein Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushtong left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 8, 2025

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snf2ye commented Jan 3, 2025 •

edited

Loading

cypress bot commented Jan 3, 2025 •

edited

Loading

snf2ye Jan 6, 2025 •

edited

Loading

snf2ye Jan 8, 2025 •

edited

Loading

s-rubenstein Jan 8, 2025 •

edited

Loading