Skip to content

Commit

Permalink
Advanced Progress Bar (ray-project#31750)
Browse files Browse the repository at this point in the history
This progress bar automatically shows progress by groupings.

Things that belong to the same parent are all put in a group.
If a group has multiple children with the same name, those are merged together into a virtual group.

These virtual groups have different visual treatment because a virtual group should not add an additional level of nesting.
  • Loading branch information
alanwguo committed Jan 31, 2023
1 parent 06197a5 commit d91d2d6
Show file tree
Hide file tree
Showing 17 changed files with 1,276 additions and 188 deletions.
2 changes: 1 addition & 1 deletion dashboard/client/src/components/PlacementGroupTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const PlacementGroupTable = ({
<TableBody>
{list.map(
({ placement_group_id, name, creator_job_id, state, stats }) => (
<TableRow>
<TableRow key={placement_group_id}>
<TableCell align="center">
<Tooltip
className={classes.idCol}
Expand Down
110 changes: 83 additions & 27 deletions dashboard/client/src/components/ProgressBar/ProgressBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import {
Typography,
} from "@material-ui/core";
import React from "react";
import { StyledTooltip } from "../Tooltip";
import { RiArrowDownSLine, RiArrowRightSLine } from "react-icons/ri";
import { HelpInfo, StyledTooltip } from "../Tooltip";

const useStyles = makeStyles((theme) =>
createStyles({
Expand Down Expand Up @@ -37,6 +38,19 @@ const useStyles = makeStyles((theme) =>
borderRadius: 4,
marginRight: theme.spacing(1),
},
hint: {
marginLeft: theme.spacing(0.5),
},
progressBarContainer: {
display: "flex",
flexDirection: "row",
alignItems: "center",
},
icon: {
width: 16,
height: 16,
marginRight: theme.spacing(1),
},
progressBarRoot: {
display: "flex",
flexDirection: "row",
Expand All @@ -52,6 +66,12 @@ const useStyles = makeStyles((theme) =>
marginRight: 1,
},
},
progressTotal: {
flex: "1 0 40px",
marginLeft: theme.spacing(1),
textAlign: "end",
whiteSpace: "nowrap",
},
}),
);

Expand All @@ -64,6 +84,10 @@ export type ProgressBarSegment = {
* Name of this segment
*/
label: string;
/**
* Text to show to explain the segment better.
*/
hint?: string;
/**
* A CSS color used to represent the segment.
*/
Expand Down Expand Up @@ -98,6 +122,20 @@ export type ProgressBarProps = {
* Whether to show the a legend as a tooltip.
*/
showTooltip?: boolean;
/**
* Whether to show the total progress to the right of the progress bar.
* Example: 5 / 20
* This should be set to the number that should be shown in the left side of the fraction.
* If this is undefined, don't show it.
*/
showTotalProgress?: number;
/**
* If true, we show an expanded icon to the left of the progress bar.
* If false, we show an unexpanded icon to the left of the progress bar.
* If undefined, we don't show any icon.
*/
expanded?: boolean;
onClick?: () => void;
};

export const ProgressBar = ({
Expand All @@ -106,6 +144,9 @@ export const ProgressBar = ({
unaccountedLabel,
showLegend = true,
showTooltip = false,
showTotalProgress,
expanded,
onClick,
}: ProgressBarProps) => {
const classes = useStyles();
const segmentTotal = progress.reduce((acc, { value }) => acc + value, 0);
Expand All @@ -118,7 +159,8 @@ export const ProgressBar = ({
...progress,
{
value: finalTotal - segmentTotal,
label: unaccountedLabel ?? "unaccounted",
label: unaccountedLabel ?? "Unaccounted",
hint: "Unaccounted tasks can happen when there are too many tasks. Ray drops older tasks to conserve memory.",
color: "#EEEEEE",
},
]
Expand All @@ -127,7 +169,7 @@ export const ProgressBar = ({
const filteredSegments = segments.filter(({ value }) => value);

return (
<div className={classes.root}>
<div className={classes.root} onClick={onClick}>
{showLegend && (
<div className={classes.legendRoot}>
<div className={classes.legendItemContainer}>
Expand All @@ -137,7 +179,7 @@ export const ProgressBar = ({
/>
<Typography>Total: {finalTotal}</Typography>
</div>
{filteredSegments.map(({ value, label, color }) => (
{filteredSegments.map(({ value, label, hint, color }) => (
<div key={label} className={classes.legendItemContainer}>
<div
className={classes.colorLegend}
Expand All @@ -146,34 +188,48 @@ export const ProgressBar = ({
<Typography>
{label}: {value}
</Typography>
{hint && <HelpInfo className={classes.hint}>{hint}</HelpInfo>}
</div>
))}
</div>
)}
<LegendTooltip
showTooltip={showTooltip}
total={finalTotal}
segments={filteredSegments}
>
<div
className={classes.progressBarRoot}
style={{
backgroundColor: segmentTotal === 0 ? "lightGrey" : "white",
}}
>
{filteredSegments.map(({ color, label, value }) => (
<span
key={label}
className={classes.segment}
style={{
flex: value,
backgroundColor: color,
}}
data-testid="progress-bar-segment"
/>
<div className={classes.progressBarContainer}>
{expanded !== undefined &&
(expanded ? (
<RiArrowDownSLine className={classes.icon} />
) : (
<RiArrowRightSLine className={classes.icon} />
))}
</div>
</LegendTooltip>
<LegendTooltip
showTooltip={showTooltip}
total={finalTotal}
segments={filteredSegments}
>
<div
className={classes.progressBarRoot}
style={{
backgroundColor: segmentTotal === 0 ? "lightGrey" : "white",
}}
>
{filteredSegments.map(({ color, label, value }) => (
<span
key={label}
className={classes.segment}
style={{
flex: value,
backgroundColor: color,
}}
data-testid="progress-bar-segment"
/>
))}
</div>
</LegendTooltip>
{showTotalProgress !== undefined && (
<div className={classes.progressTotal}>
{showTotalProgress} / {finalTotal}
</div>
)}
</div>
</div>
);
};
Expand Down
2 changes: 1 addition & 1 deletion dashboard/client/src/components/TaskTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ const TaskTable = ({
start_time_ms,
end_time_ms,
}) => (
<TableRow>
<TableRow key={task_id}>
<TableCell align="center">
<Tooltip
className={classes.idCol}
Expand Down
4 changes: 2 additions & 2 deletions dashboard/client/src/components/TitleCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ const useStyles = makeStyles((theme) => ({
const TitleCard = ({
title,
children,
}: PropsWithChildren<{ title: ReactNode | string }>) => {
}: PropsWithChildren<{ title?: ReactNode | string }>) => {
const classes = useStyles();
return (
<Paper className={classes.card}>
<div className={classes.title}>{title}</div>
{title && <div className={classes.title}>{title}</div>}
<div className={classes.body}>{children}</div>
</Paper>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import { Table, TableBody } from "@material-ui/core";
import { ThemeProvider } from "@material-ui/styles";
import { render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import React, { PropsWithChildren } from "react";
import { lightTheme } from "../../../theme";
import { TypeTaskType } from "../../../type/task";
import { AdvancedProgressBarSegment } from "./AdvancedProgressBar";

const Wrapper = ({ children }: PropsWithChildren<{}>) => {
return (
<ThemeProvider theme={lightTheme}>
<Table>
<TableBody>{children}</TableBody>
</Table>
</ThemeProvider>
);
};

describe("AdvancedProgressBarSegment", () => {
it("renders without children", async () => {
expect.assertions(2);
render(
<AdvancedProgressBarSegment
jobProgressGroup={{
name: "group 1",
key: "group1",
progress: {
numFinished: 1,
numRunning: 9,
},
children: [],
type: TypeTaskType.ACTOR_CREATION_TASK,
}}
/>,
{ wrapper: Wrapper },
);
await screen.findByText(/group 1/);
expect(screen.getByText(/1 \/ 10/)).toBeVisible();
expect(screen.getByTitle("Expand").parentElement).not.toBeVisible();
});

it("renders with children", async () => {
expect.assertions(7);
const user = userEvent.setup();

render(
<AdvancedProgressBarSegment
jobProgressGroup={{
name: "group 1",
key: "group1",
progress: {
numFinished: 1,
numRunning: 9,
},
children: [
{
name: "child",
key: "child",
progress: {
numFinished: 1,
},
children: [],
type: TypeTaskType.NORMAL_TASK,
},
],
type: TypeTaskType.ACTOR_CREATION_TASK,
}}
/>,
{ wrapper: Wrapper },
);
await screen.findByText(/group 1/);
expect(screen.getByTitle("Expand").parentElement).toBeVisible();
expect(screen.getByText(/^1 \/ 10$/)).toBeVisible();
await user.click(screen.getByTitle("Expand"));
await screen.findByText(/child/);
screen.getByText(/child/);
expect(screen.getByTitle("Collapse").parentElement).toBeVisible();
expect(screen.getAllByTitle("Expand")).toHaveLength(1); // There should only be one for the child segment
expect(screen.getByText(/^1 \/ 1$/)).toBeVisible();
await user.click(screen.getByTitle("Collapse"));
expect(screen.queryByText(/child/)).toBeNull();
expect(screen.queryByText(/^1 \/ 1$/)).toBeNull();
});

it("renders with GROUP and children", async () => {
expect.assertions(12);
const user = userEvent.setup();

render(
<AdvancedProgressBarSegment
jobProgressGroup={{
name: "group 1",
key: "group1",
progress: {
numFinished: 3,
numRunning: 7,
},
type: "GROUP",
children: [
{
name: "child",
key: "child",
progress: {
numFinished: 3,
},
type: TypeTaskType.NORMAL_TASK,
children: [
{
name: "grandchild",
key: "grandchild",
progress: {
numFinished: 1,
},
type: TypeTaskType.NORMAL_TASK,
children: [],
},
],
},
],
}}
/>,
{ wrapper: Wrapper },
);
await screen.findByText(/group 1/);
expect(screen.getByTitle("Expand").parentElement).toBeVisible();
expect(screen.getByText(/^3 \/ 10$/)).toBeVisible();
await user.click(screen.getByTitle("Expand"));
await screen.findByText(/child/);
screen.getByText(/child/);
expect(screen.getByTitle("Collapse group").parentElement).toBeVisible();
expect(screen.getAllByTitle("Expand")).toHaveLength(1); // There should only be one for the child segment
expect(screen.getByText(/^3 \/ 3$/)).toBeVisible();
await user.click(screen.getByTitle("Expand"));
await screen.findByText(/grandchild/);
expect(screen.getByTitle("Collapse group").parentElement).toBeVisible();
expect(screen.getByTitle("Collapse").parentElement).toBeVisible(); // Collapse on the child segment
expect(screen.getAllByTitle("Expand")).toHaveLength(1); // There should only be one for the grand child segment
expect(screen.getByText(/^1 \/ 1$/)).toBeVisible();
await user.click(screen.getByTitle("Collapse group"));
expect(screen.getByText(/^3 \/ 10$/)).toBeVisible();
expect(screen.queryByText(/^3 \/ 3$/)).toBeNull();
expect(screen.queryByText(/^1 \/ 1$/)).toBeNull();
});
});
Loading

0 comments on commit d91d2d6

Please sign in to comment.