[Dashboard] Upgrade to MUIv5 and dependency updates #45789

liuxsh9 · 2024-06-07T01:58:42Z

Why are these changes needed?

This PR replaces the current makeStyles in the Dashboard with the MUIv5 recommended styled approach, thereby removing the @mui/styles transitional package, which facilitates future upgrades and expansions. Additionally, the React framework has been upgraded to version 18.3.0, and lowlight has been upgraded to version 2.9.0.

Related issue number

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

anyscalesam · 2024-06-14T21:08:51Z

@aslonnie @alanwguo can PTAL and see if this is safe to merge; nice to have an upgrade in... and if our CI and release test pass I don't think there is any harm in keeping up to date.

alanwguo

wow! thanks for tackling this.

We've been using sx props internally and in the ray dashboard code base for new components that we create. Can you update the PR to use sx props for all the cases where we can easily do it.

Like when using Typography or Paper or other mui components

alanwguo · 2024-06-14T21:19:54Z

dashboard/client/src/common/CollapsibleSection.tsx

-    },
-  }),
-);
+const TitleTypography = styled(Typography)(({ theme }) => ({


for something like Typography which accepts sx, can we use sx props instead of styled?

No problem, we can adjust to using sx as the default.

Have replaced them with sx, please take a look.

aslonnie · 2024-06-16T03:53:48Z

I really have no opinion / saying on this.. this is like pure js land stuff, will leave this to @alanwguo and dashboard / observability team

if we need to upgrade npm or something to make this happen, let me know.

aslonnie · 2024-06-16T03:55:02Z

side note: this PR looks rather big, might be easier to get it merge if this can be done in stages, though I do not really know technically how to make it happen.

Updated the React framework to version 18.3.0. Update lowlight packages and remove unused packages. Signed-off-by: liuxsh9 <[email protected]>

Signed-off-by: liuxsh9 <[email protected]>

Updated the React framework to version 18.3.0. Update lowlight packages and remove unused packages. Signed-off-by: liuxsh9 <[email protected]>

Signed-off-by: liuxsh9 <[email protected]>

…les transition package. Upgrade the react and lowlight modules. Signed-off-by: liuxsh9 <[email protected]>

alanwguo · 2024-06-24T16:15:01Z

dashboard/client/src/common/CodeDialogButton/CodeDialogButton.tsx

@@ -111,13 +114,15 @@ export const CodeDialogButtonWithPreview = ({
  const buttonTextToPass = buttonText ?? "Expand";

  return (
-    <div className={classNames(classes.root, className)}>
-      <span className={classes.previewText}>{codeText}</span>
+    <Box className={className} sx={Object.assign({}, styles.root, sx)}>


Suggested change

<Box className={className} sx={Object.assign({}, styles.root, sx)}>

<Box className={className} sx={[styles.root, sx]}>

Please use array syntax instead

Sure! Made a minor adjustment to sx={[styles.root, ...(Array.isArray(sx) ? sx : [sx])]} to ensure the type matches.

alanwguo · 2024-06-24T16:16:07Z

dashboard/client/src/common/CodeDialogButton/CodeDialogButton.tsx

  ...props
 }: CodeDialogButtonWithPreviewProps) => {
-  const classes = useCodeDialogButtonWithPreviewStyles();
+  const styles = useCodeDialogButtonWithPreviewStyles(useTheme());


There shouldn't be a need to create an object like this.

When using sx props, you can also easily get access to the theme by passing a function into sx.

In most cases, I would prefer if we inlined styles instead of creating a separate styles object, but this is not a blocker.

Understood, I've removed the redundant object and directly passed the function instead. Additionally, converted some parts to use inlined styles, while leaving the rest as is for readability considerations.

alanwguo · 2024-06-24T16:25:14Z

dashboard/client/src/common/JobStatus.tsx

-      )}
+    <Box
+      component={RiLoader4Line}
+      sx={Object.assign({}, styles.icon(small), styles.iconRunning, sx)}


please use array syntax

alanwguo · 2024-06-24T16:26:15Z

dashboard/client/src/common/JobStatus.tsx

+        <Box
+          component={RiCheckboxCircleFill}
+          className={className}
+          sx={Object.assign({}, styles.icon(small), styles.colorSuccess, sx)}


array syntax

Signed-off-by: liuxsh9 <[email protected]>

liuxsh9 · 2024-06-26T07:15:12Z

Thanks for the suggestions! We've completed the corresponding modifications. How does the new version look? @alanwguo

alanwguo

I went around the UI looking for visual bugs. There was only one that I found.

Thanks for the effort. I think the PR is almost ready.

alanwguo · 2024-06-26T16:55:26Z

dashboard/client/src/common/CollapsibleSection.tsx

+              <Box
+                component={RiArrowDownSLine}
+                sx={(theme) => ({
+                  marginRight: theme.spacing(1),


Suggested change

marginRight: theme.spacing(1),

marginRight: 1,

nit: sx automatically uses theme spacing values when passing in numbers for margins, padding, border, etc

Got it! Have made the changes.

alanwguo · 2024-06-26T20:55:42Z

dashboard/client/src/common/CodeDialogButton/CodeDialogButton.tsx

-const useCodeDialogButtonWithPreviewStyles = makeStyles((theme) =>
-  createStyles({
-    root: {
-      display: "flex",
-      flexWrap: "nowrap",
-      flexDirection: "row",
-      gap: theme.spacing(1),
-    },
-    previewText: {
-      display: "block",
-      whiteSpace: "nowrap",
-      overflow: "hidden",
-      textOverflow: "ellipsis",
-      flex: 1,
-    },
-  }),
-);
-


I noticed this component has some missing styles after your PR:

You can repro this by going to the jobs page -> job detail -> runtime environment (view) button

Signed-off-by: liuxsh9 <[email protected]>

alanwguo

looks great! Thanks. Just some small non-blockers

alanwguo · 2024-06-28T17:24:23Z

dashboard/client/src/components/LogView/LogVirtualView.tsx

@@ -17,6 +16,8 @@ import "./github.css";
 import "./index.css";
 import { MAX_LINES_FOR_LOGS } from "../../service/log";

+lowlight.registerLanguage("prolog", prolog);


curious what this is for?

After lowlight 2.0, it introduced three import modes: core, common (default), and all. To prioritize simplicity and extensibility, we opted for the default common mode, which auto-registers 30+ languages. However, prolog is not included in the list. Therefore, we need to manually register it here.
wooorm/lowlight@95cd2f6

alanwguo · 2024-06-28T17:25:20Z

dashboard/client/src/components/TitleCard.tsx

+      sx={{
+        padding: 2,
+        paddingTop: 1.5,
+        margin: (theme) => theme.spacing(2, 1),


nit:

Suggested change

margin: (theme) => theme.spacing(2, 1),

marginX: 1,

marginY: 2,

Good to use a uniform format, have modified them.

Signed-off-by: liuxsh9 <[email protected]>

liuxsh9 · 2024-06-29T09:10:19Z

dashboard/client/package.json

@@ -53,6 +48,11 @@
  "resolutions": {
    "@types/react": "16.9.26"
  },
+  "jest": {
+    "transformIgnorePatterns": [


The new version of lowlight has extended its syntax to TypeScript, which requires additional configuration when using Jest for testing. The official recommendation is to adapt to the following scheme:

If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript

To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.

We are currently adding configuration here, is it suitable?

I can't find any thing online about this. What happens if you don't include this?

CI will fail, the error message:

… in the object. Signed-off-by: liuxsh9 <[email protected]>

alanwguo

I just did another round of testing. Looks great!

Thanks!!

liuxsh9 force-pushed the dashboard_mui5 branch from 91a631a to b5027ef Compare June 7, 2024 03:20

anyscalesam requested review from alanwguo and aslonnie June 14, 2024 21:08

anyscalesam assigned aslonnie Jun 14, 2024

alanwguo reviewed Jun 14, 2024

View reviewed changes

aslonnie removed their request for review June 16, 2024 03:53

aslonnie assigned alanwguo and unassigned aslonnie Jun 16, 2024

liuxsh9 added 5 commits June 24, 2024 09:54

Replace makeStyles with styled in MUIv5.

1edf57c

Updated the React framework to version 18.3.0. Update lowlight packages and remove unused packages. Signed-off-by: liuxsh9 <[email protected]>

Format Code

7c809cb

Signed-off-by: liuxsh9 <[email protected]>

Replace makeStyles with styled in MUIv5.

1302e60

Updated the React framework to version 18.3.0. Update lowlight packages and remove unused packages. Signed-off-by: liuxsh9 <[email protected]>

Format Code

225af33

Signed-off-by: liuxsh9 <[email protected]>

Fix some warnings

575f8cb

Signed-off-by: liuxsh9 <[email protected]>

liuxsh9 force-pushed the dashboard_mui5 branch from ce63bec to 575f8cb Compare June 24, 2024 02:40

Use the sx prop of MUI v5 to apply the styles and remove the @mui/sty…

28fbb63

…les transition package. Upgrade the react and lowlight modules. Signed-off-by: liuxsh9 <[email protected]>

alanwguo reviewed Jun 24, 2024

View reviewed changes

liuxsh9 added 2 commits June 25, 2024 14:48

Replace the style combination method with array syntax to fix the error.

40e85c7

Signed-off-by: liuxsh9 <[email protected]>

Inline styles using sx props

bf5e4b1

Signed-off-by: liuxsh9 <[email protected]>

alanwguo reviewed Jun 26, 2024

View reviewed changes

Fix errors and abbreviate styles such as padding

94a28b4

Signed-off-by: liuxsh9 <[email protected]>

alanwguo approved these changes Jun 28, 2024

View reviewed changes

alanwguo added the go add ONLY when ready to merge, run all tests label Jun 28, 2024

liuxsh9 added 2 commits June 29, 2024 10:27

abbreviate styles such as padding, margin and gap

ebb9248

Signed-off-by: liuxsh9 <[email protected]>

Fix errors that occurred during testing

71cf33b

Signed-off-by: liuxsh9 <[email protected]>

liuxsh9 commented Jun 29, 2024

View reviewed changes

Remove style testing. When using sx prop, there is no style attribute…

c878dcd

… in the object. Signed-off-by: liuxsh9 <[email protected]>

alanwguo approved these changes Jul 2, 2024

View reviewed changes

rkooo567 merged commit cab9059 into ray-project:master Jul 2, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dashboard] Upgrade to MUIv5 and dependency updates #45789

[Dashboard] Upgrade to MUIv5 and dependency updates #45789

liuxsh9 commented Jun 7, 2024

anyscalesam commented Jun 14, 2024

alanwguo left a comment

alanwguo Jun 14, 2024

liuxsh9 Jun 19, 2024

liuxsh9 Jun 24, 2024

aslonnie commented Jun 16, 2024

aslonnie commented Jun 16, 2024

alanwguo Jun 24, 2024

liuxsh9 Jun 25, 2024

alanwguo Jun 24, 2024

alanwguo Jun 24, 2024

liuxsh9 Jun 26, 2024

alanwguo Jun 24, 2024

liuxsh9 Jun 25, 2024

alanwguo Jun 24, 2024

liuxsh9 Jun 25, 2024

liuxsh9 commented Jun 26, 2024

alanwguo left a comment

alanwguo Jun 26, 2024

liuxsh9 Jun 27, 2024

alanwguo Jun 26, 2024

liuxsh9 Jun 27, 2024

alanwguo left a comment

alanwguo Jun 28, 2024

liuxsh9 Jun 29, 2024 •

edited

Loading

alanwguo Jun 28, 2024

liuxsh9 Jun 29, 2024

liuxsh9 Jun 29, 2024

alanwguo Jul 1, 2024

liuxsh9 Jul 2, 2024 •

edited

Loading

alanwguo left a comment

	<Box className={className} sx={Object.assign({}, styles.root, sx)}>
	<Box className={className} sx={[styles.root, sx]}>

	margin: (theme) => theme.spacing(2, 1),
	marginX: 1,
	marginY: 2,

[Dashboard] Upgrade to MUIv5 and dependency updates #45789

[Dashboard] Upgrade to MUIv5 and dependency updates #45789

Conversation

liuxsh9 commented Jun 7, 2024

Why are these changes needed?

Related issue number

Checks

anyscalesam commented Jun 14, 2024

alanwguo left a comment

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

aslonnie commented Jun 16, 2024

aslonnie commented Jun 16, 2024

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

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

Choose a reason for hiding this comment

liuxsh9 commented Jun 26, 2024

alanwguo left a comment

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

Choose a reason for hiding this comment

alanwguo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxsh9 Jun 29, 2024 • 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

Choose a reason for hiding this comment

liuxsh9 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

alanwguo left a comment

Choose a reason for hiding this comment

liuxsh9 Jun 29, 2024 •

edited

Loading

liuxsh9 Jul 2, 2024 •

edited

Loading