Skip to content

Commit

Permalink
[ui] Place run Gantt chart arrows between boxes (#20987)
Browse files Browse the repository at this point in the history
Fixes #19331

## Summary & Motivation

A user in #19331 flagged that our Gantt chart line rendering is
non-ideal for fan-out scenarios. Lines on the Gantt chart always go over
THEN down, which means they are always cutting through a row of boxes.
It's been like this for as long as I can remember, and I don't recall
why we chose to do it this way. I think that we may have changed the
rendering algorithm to make better use of vertical space by allowing
boxes to appear on the same lines, and when we did that, we didn't
update the line rendering.

The new version moves the vertical part of the line so it appears just
after the first box and before the next layer of boxes, which results in
cleaner rendering just about all the time I think.

I also changed the line colors to match the DAG UI, because the hover
color change wasn't very obvious.

Before:
<img width="612" alt="image"
src="https://github.com/dagster-io/dagster/assets/1037212/3a6187dc-802b-45b8-811c-2d1486b36ef1">

<img width="808" alt="image"
src="https://github.com/dagster-io/dagster/assets/1037212/0a0c3bff-5a3a-413c-a18b-6c60c15d246f">


After:

<img width="583" alt="image"
src="https://github.com/dagster-io/dagster/assets/1037212/d8b5191b-168d-436a-b262-9825ce233567">

<img width="771" alt="image"
src="https://github.com/dagster-io/dagster/assets/1037212/f7325a5a-252c-4ea9-bee5-a580e5fc54a8">


## How I Tested These Changes

Just tested manually and using our storybook coverage of this component

Co-authored-by: bengotow <[email protected]>
  • Loading branch information
bengotow and bengotow authored Apr 3, 2024
1 parent 3d813e4 commit b350424
Showing 1 changed file with 57 additions and 13 deletions.
70 changes: 57 additions & 13 deletions js_modules/dagster-ui/packages/ui-core/src/gantt/GanttChart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -590,20 +590,14 @@ const boundsForLine = (a: GanttChartBox, b: GanttChartBox): Bounds => {
const bIsDot = b.width === BOX_DOT_WIDTH_CUTOFF;
const bCenterY = bIsDot ? BOX_DOT_MARGIN_Y + BOX_DOT_SIZE / 2 : BOX_HEIGHT / 2;

const straight = b.y === a.y;

// Line comes out of the center of the right side of the box
const minX = Math.min(a.x + a.width, b.x + b.width);
const minY = TOP_INSET + (straight ? a.y * BOX_HEIGHT + aCenterY : a.y * BOX_HEIGHT + aCenterY);
const minY = TOP_INSET + a.y * BOX_HEIGHT + aCenterY;

// Line ends on the center left edge of the box if it is on the
// same line, or drops into the top center of the box if it's below.
const maxX = straight
? Math.max(a.x, b.x)
: Math.max(a.x + a.width / 2, b.x + (bIsDot ? BOX_DOT_SIZE : b.width) / 2);
const maxY = straight
? TOP_INSET + b.y * BOX_HEIGHT + bCenterY
: TOP_INSET + b.y * BOX_HEIGHT + (bIsDot ? BOX_DOT_MARGIN_Y : BOX_MARGIN_Y);
const maxX = Math.max(a.x, b.x);
const maxY = TOP_INSET + b.y * BOX_HEIGHT + bCenterY;

return {minX, minY, maxX, maxY};
};
Expand All @@ -629,10 +623,49 @@ const GanttLine = React.memo(
depNotDrawn: boolean;
} & Bounds) => {
const border = `${LINE_SIZE}px ${dotted ? 'dotted' : 'solid'} ${
darkened ? Colors.accentGray() : Colors.accentGrayHover()
darkened ? Colors.lineageEdgeHighlighted() : Colors.lineageEdge()
}`;

const maxXAvoidingOverlap = maxX + (depIdx % 10) * LINE_SIZE;
if (depNotDrawn) {
return (
<div
className="line"
style={{
height: 1,
left: minX,
width: 50,
top: minY - 1,
borderTop: border,
zIndex: darkened ? 100 : 1,
}}
/>
);
}
if (minY === maxY) {
return (
<div
className="line"
style={{
height: 1,
left: minX,
width: maxX - minX,
top: minY - 1,
borderTop: border,
zIndex: darkened ? 100 : 1,
}}
/>
);
}

// When drawing a line that has a vertical segment, position the vertical segment
// in the space after the first box before the next box begins. This is ideal because
// there can be "marker-dot" and "marker-whiskers" elements before the next box, and
// the line looks better going straight through those.
//
// The idx lgoic here ensures that outgoing lines from a single box "stack" rather than
// layering on top of each other.
//
const verticalLineX = minX + BOX_SPACING_X * 0.4 + (depIdx % 10) * LINE_SIZE;

return (
<>
Expand All @@ -641,7 +674,7 @@ const GanttLine = React.memo(
style={{
height: 1,
left: minX,
width: depNotDrawn ? 50 : maxXAvoidingOverlap - minX,
width: verticalLineX - minX,
top: minY - 1,
borderTop: border,
zIndex: darkened ? 100 : 1,
Expand All @@ -652,14 +685,25 @@ const GanttLine = React.memo(
className="line"
style={{
width: 1,
left: maxXAvoidingOverlap,
left: verticalLineX,
top: minY - LINE_SIZE / 2,
height: maxY - minY,
borderRight: border,
zIndex: darkened ? 100 : 1,
}}
/>
)}
<div
className="line"
style={{
height: 1,
left: verticalLineX,
width: maxX - verticalLineX,
top: maxY - 1,
borderTop: border,
zIndex: darkened ? 100 : 1,
}}
/>
</>
);
},
Expand Down

1 comment on commit b350424

@github-actions
Copy link

Choose a reason for hiding this comment

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

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-o2xn21809-elementl.vercel.app

Built with commit b350424.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.