Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 38 commits into from
Jan 14, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
4e16bbd
HomeView - converted with copilot
snf2ye Jan 3, 2025
9ff762b
DatasetView - upgrade by copilot
snf2ye Jan 3, 2025
08016b8
DatasetView - Copilot decided to go rouge and just add a title??
snf2ye Jan 3, 2025
841ade9
DatasetView: Copilot dropped the mixins css
snf2ye Jan 3, 2025
6eab2ee
SnapshotView: Prep for copilot changes
snf2ye Jan 3, 2025
390d32a
Snapshot View: Update by copilot
snf2ye Jan 3, 2025
49d2f7c
SnapshotView: copilot dropped the mixins css
snf2ye Jan 3, 2025
aa6caf9
JobView - prep for copilot - remove unused styles
snf2ye Jan 3, 2025
ef996c4
JobView - upgrade by copilot
snf2ye Jan 3, 2025
2612ace
JobView: cleanup
snf2ye Jan 3, 2025
ac262ac
SnapshotAccessRequestTable
snf2ye Jan 3, 2025
fb6d70e
DatasetOverview: Remove unused styles
snf2ye Jan 3, 2025
cd86d3f
DatasetOverview - Copilot update
snf2ye Jan 3, 2025
02041ab
DatasetOverviewPanel - update with copilot
snf2ye Jan 3, 2025
9ce17e2
DatasetOverviewPanel.jsx - set the Drawer to temporary so it doesn't …
snf2ye Jan 3, 2025
817e72f
Index
snf2ye Jan 3, 2025
4ee461c
DatasetOverviewPanel.jsx - clean up references
snf2ye Jan 7, 2025
5b7460a
DatasetAccess - remove styles, not referenced in component
snf2ye Jan 7, 2025
bc1313e
UserList - remove unused styles
snf2ye Jan 7, 2025
849618b
UserList - update by copilot
snf2ye Jan 7, 2025
2358483
JobView - switch back to more specific type
snf2ye Jan 8, 2025
5e3e9c1
HomeView
snf2ye Jan 9, 2025
cc59308
UserList - updated by claude copilot
snf2ye Jan 9, 2025
8570011
DatasetView
snf2ye Jan 9, 2025
89aabed
SnapshotView
snf2ye Jan 9, 2025
d676ced
Job View
snf2ye Jan 9, 2025
b6062e3
DatasetOverview
snf2ye Jan 9, 2025
552e01b
DatasetOverviewPanel
snf2ye Jan 10, 2025
640e7b1
Remove styles
snf2ye Jan 10, 2025
5df0310
SnapshotAccessRequestTable.tsx
snf2ye Jan 10, 2025
504225e
Use styled from @mui/system, move mixins to be styled components
snf2ye Jan 10, 2025
f3d2436
Fix bug in DatasetOverviewPanel.jsx
snf2ye Jan 10, 2025
a5f2e25
needed component=span
snf2ye Jan 10, 2025
1fe3370
Getting typescript to recognize that we're using our extended CustomT…
snf2ye Jan 11, 2025
5148d72
Move to common component
snf2ye Jan 14, 2025
2ceceb6
On second thought, let's remove containerWidth
snf2ye Jan 14, 2025
ad32ffd
Move away from shorthand to more verbose css styling
snf2ye Jan 14, 2025
335319e
Add px; update one more test reference
snf2ye Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 53 additions & 69 deletions src/components/DatasetView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,15 @@
import React, { Dispatch } from 'react';
import { connect } from 'react-redux';
import { Action } from 'redux';
import { WithStyles, withStyles } from '@mui/styles';
import { Box } from '@mui/material';
import { getDatasets, addDatasetPolicyMember } from 'actions/index';
import { DatasetSummaryModel } from 'generated/tdr';
import { CustomTheme } from '@mui/material';
import { TdrState } from 'reducers';
import { OrderDirectionOptions } from 'reducers/query';
import DatasetTable from './table/DatasetTable';

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

const styles = (theme: CustomTheme) => ({
wrapper: {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

display: 'flex',
justifyContent: 'center',
marginTop: '1em',
},
width: {
...theme.mixins.containerWidth,
},
title: {
color: theme.palette.primary.main,
fontSize: '54px',
lineHeight: '66px',
paddingBottom: theme.spacing(8),
},
});

interface IProps extends WithStyles<typeof styles> {
interface IProps {
datasets: Array<DatasetSummaryModel>;
datasetRoleMaps: { [key: string]: Array<string> };
datasetsCount: number;
Expand All @@ -40,56 +21,59 @@ interface IProps extends WithStyles<typeof styles> {
userEmail: string;
}

const DatasetView = withStyles(styles)(
({
classes,
datasets,
datasetRoleMaps,
datasetsCount,
dispatch,
filteredDatasetsCount,
loading,
searchString,
refreshCnt,
userEmail,
}: IProps) => {
const handleFilterDatasets = (
limit: number,
offset: number,
sort: string,
sortDirection: OrderDirectionOptions,
search: string,
) => {
dispatch(getDatasets(limit, offset, sort, sortDirection, search));
};
function DatasetView({
datasets,
datasetRoleMaps,
datasetsCount,
dispatch,
filteredDatasetsCount,
loading,
searchString,
refreshCnt,
userEmail,
}: IProps) {
const handleFilterDatasets = (
limit: number,
offset: number,
sort: string,
sortDirection: OrderDirectionOptions,
search: string,
) => {
dispatch(getDatasets(limit, offset, sort, sortDirection, search));
};

const handleMakeSteward = (datasetId: string) => {
dispatch(addDatasetPolicyMember(datasetId, userEmail, DatasetRoles.STEWARD));
};
const handleMakeSteward = (datasetId: string) => {
dispatch(addDatasetPolicyMember(datasetId, userEmail, DatasetRoles.STEWARD));
};

return (
<div className={classes.wrapper}>
<div className={classes.width}>
<div>
{datasets && (
<DatasetTable
datasets={datasets}
datasetRoleMaps={datasetRoleMaps}
datasetsCount={datasetsCount}
handleFilterDatasets={handleFilterDatasets}
handleMakeSteward={handleMakeSteward}
filteredDatasetsCount={filteredDatasetsCount}
searchString={searchString}
loading={loading}
refreshCnt={refreshCnt}
/>
)}
</div>
</div>
</div>
);
},
);
return (
<Box
sx={{
display: 'flex',
justifyContent: 'center',
marginTop: '1em',
}}
>
<Box sx={{ width: '100%' }}>
<Box>
{datasets && (
<DatasetTable
datasets={datasets}
datasetRoleMaps={datasetRoleMaps}
datasetsCount={datasetsCount}
handleFilterDatasets={handleFilterDatasets}
handleMakeSteward={handleMakeSteward}
filteredDatasetsCount={filteredDatasetsCount}
searchString={searchString}
loading={loading}
refreshCnt={refreshCnt}
/>
)}
</Box>
</Box>
</Box>
);
}

function mapStateToProps(state: TdrState) {
return {
Expand Down
130 changes: 43 additions & 87 deletions src/components/HomeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,10 @@ import React, { Dispatch, useState } from 'react';
import { connect } from 'react-redux';
import { Link } from 'react-router-dom';
import { Action } from 'redux';

import { Button } from '@mui/material';
import { Button, Box } from '@mui/material';
import { styled } from '@mui/material/styles';
import { AddCircle, Refresh } from '@mui/icons-material';
import { createStyles, WithStyles, withStyles } from '@mui/styles';
import { CustomTheme } from '@mui/material/styles';
import { RouterLocation, RouterRootState } from 'connected-react-router';
import { LocationState } from 'history';
import {
getSnapshotAccessRequests,
refreshDatasets,
Expand All @@ -18,76 +15,42 @@ import {
} from 'src/actions';
import { TdrState } from 'reducers';
import { useOnMount } from 'libs/utils';
import { LocationState } from 'history';
import DatasetView from './DatasetView';
import SnapshotView from './SnapshotView';
import JobView from './JobView';
import SearchTable from './table/SearchTable';
import SnapshotAccessRequestView from './SnapshotAccessRequestView';

const styles = (theme: CustomTheme) =>
createStyles({
pageRoot: {
padding: '16px 24px',
display: 'flex',
flexDirection: 'column',
height: '100%',
},
header: {
alignItems: 'center',
color: theme.typography.color,
display: 'flex',
fontWeight: 500,
fontSize: 16,
height: 21,
letterSpacing: 1,
},
jadeTableSpacer: {
paddingBottom: theme.spacing(12),
},
jadeLink: {
...theme.mixins.jadeLink,
float: 'right',
fontSize: 16,
fontWeight: 500,
height: 20,
letterSpacing: 0.3,
paddingLeft: theme.spacing(4),
paddingTop: theme.spacing(4),
},
title: {
color: theme.palette.secondary.dark,
fontSize: '1.5rem',
fontWeight: 700,
flex: '1 1 0',
'padding-right': '2em',
display: 'flex',
},
titleText: {
width: '150px',
},
titleAndSearch: {
display: 'flex',
'margin-top': '1.25em',
'margin-bottom': '1.25em',
},
headerButton: {
padding: 10,
marginLeft: theme.spacing(2),
height: '45px',
textTransform: 'none',
},
buttonIcon: {
marginRight: 6,
fontSize: '1.5rem',
},
});
const RootContainer = styled(Box)({
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this approach - very clean.

padding: '16px 24px',
display: 'flex',
flexDirection: 'column',
height: '100%',
});

const TitleContainer = styled(Box)(({ theme }) => ({
color: theme.palette.secondary.dark,
fontSize: '1.5rem',
fontWeight: 700,
flex: '1 1 0',
paddingRight: '2em',
display: 'flex',
}));

const HeaderButton = styled(Button)({
padding: '10px',
marginLeft: '16px',
height: '45px',
textTransform: 'none',
});

interface IProps extends WithStyles<typeof styles> {
interface IProps {
dispatch: Dispatch<Action>;
location: RouterLocation<LocationState>;
}

function HomeView({ classes, dispatch, location }: IProps) {
function HomeView({ dispatch, location }: IProps) {
const [searchString, setSearchString] = useState('');
const prefixMatcher = /\/[^/]*/;
const tabValue = prefixMatcher.exec(location.pathname)?.[0];
Expand Down Expand Up @@ -115,48 +78,41 @@ function HomeView({ classes, dispatch, location }: IProps) {
refresh = () => dispatch(refreshSnapshotAccessRequests());
}
const refreshButton = (
<Button
<HeaderButton
aria-label="refresh page"
size="medium"
className={classes.headerButton}
onClick={refresh}
variant="outlined"
startIcon={<Refresh />}
>
Refresh
</Button>
</HeaderButton>
);
const pageHeader =
tabValue === '/datasets' ? (
<div className={classes.title}>
<span className={classes.titleText}>{pageTitle}</span>
<TitleContainer>
<Box sx={{ width: '150px' }}>{pageTitle}</Box>
{refreshButton}
<Link to="datasets/new" data-cy="create-dataset-link">
<Button
className={classes.headerButton}
color="primary"
variant="outlined"
disableElevation
size="medium"
>
<AddCircle className={classes.buttonIcon} /> Create Dataset
</Button>
<HeaderButton color="primary" variant="outlined" disableElevation size="medium">
<AddCircle sx={{ marginRight: '6px', fontSize: '1.5rem' }} /> Create Dataset
</HeaderButton>
</Link>
</div>
</TitleContainer>
) : (
<div className={classes.title}>
<span className={classes.titleText}>{pageTitle}</span>
<TitleContainer>
<Box sx={{ width: '150px' }}>{pageTitle}</Box>
{refreshButton}
</div>
</TitleContainer>
);

useOnMount(() => {
dispatch(getSnapshotAccessRequests());
});

return (
<div className={classes.pageRoot}>
<div className={classes.titleAndSearch}>
<RootContainer>
<Box sx={{ display: 'flex', marginTop: '1.25em', marginBottom: '1.25em' }}>
{pageHeader}
{searchable && (
<SearchTable
Expand All @@ -165,9 +121,9 @@ function HomeView({ classes, dispatch, location }: IProps) {
clearSearchString={() => setSearchString('')}
/>
)}
</div>
</Box>
{tableValue}
</div>
</RootContainer>
);
}

Expand All @@ -177,4 +133,4 @@ function mapStateToProps(state: TdrState & RouterRootState) {
};
}

export default connect(mapStateToProps)(withStyles(styles)(HomeView));
export default connect(mapStateToProps)(HomeView);
Loading
Loading