Skip to content

Commit

Permalink
[dagit] Stop foreign node links from creating sprawling asset graphs (#…
Browse files Browse the repository at this point in the history
…11588)

### Summary & Motivation

This is a fix for one of the most pressing issues @braunjj identified in
his asset graph explorations -- we weren't capping the length of
external graph links on the Asset Graph, so they could easily result in
horizontal expansion of the graph.

This PR limits them to 10 characters, which is enough that two can be
shown side-by-side above an asset node without increasing the graph
width. We could potentially just show the icon or redesign these futher,
but this should be a good incremental improvement.

In this PR I also simplified some of the asset node sizing. Rather than
trying to vary the size of the boxes a bit based on their names, we make
them the same width all the time. This results in better renderings
because it's more likely that the assets fit into a nice grid. It also
allows us to fix a bug in the "minimal" rendering where occasionally the
labels would overflow their containers.

### How I Tested These Changes

Before:
![Screen Shot 2023-01-09 at 2 34 33
PM](https://user-images.githubusercontent.com/1037212/211403295-12b798f1-d9a6-4c78-9676-f1de0c179ff9.png)

After:
![Screen Shot 2023-01-09 at 2 34 20
PM](https://user-images.githubusercontent.com/1037212/211403292-9844b1df-1b54-4a5b-8b63-431705a9d691.png)

Co-authored-by: bengotow <bgotow@elementl.com>
  • Loading branch information
bengotow and bengotow committed Jan 12, 2023
1 parent af1707e commit d62bd97
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ export const AssetNodeMinimal: React.FC<{
</div>

<MinimalName style={{fontSize: 30}} $isSource={isSource}>
{withMiddleTruncation(displayName, {maxLength: 17})}
{withMiddleTruncation(displayName, {maxLength: 14})}
</MinimalName>
</MinimalAssetNodeBox>
</MinimalAssetNodeContainer>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,21 @@ import {Colors, Icon, FontFamily} from '@dagster-io/ui';
import React from 'react';
import styled from 'styled-components/macro';

import {withMiddleTruncation} from '../app/Util';

import {displayNameForAssetKey} from './Utils';
import {ASSET_LINK_NAME_MAX_LENGTH} from './layout';

export const AssetNodeLink: React.FC<{
assetKey: {path: string[]};
}> = React.memo(({assetKey}) => (
<AssetNodeLinkContainer>
<Icon name="open_in_new" color={Colors.Link} />
<span className="label">{displayNameForAssetKey(assetKey)}</span>
<span className="label">
{withMiddleTruncation(displayNameForAssetKey(assetKey), {
maxLength: ASSET_LINK_NAME_MAX_LENGTH,
})}
</span>
</AssetNodeLinkContainer>
));

Expand Down
26 changes: 8 additions & 18 deletions js_modules/dagit/packages/core/src/asset-graph/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import groupBy from 'lodash/groupBy';

import {IBounds, IPoint} from '../graph/common';

import {GraphData, GraphNode, GraphId, displayNameForAssetKey} from './Utils';
import {GraphData, GraphNode, GraphId} from './Utils';

export interface AssetLayout {
id: GraphId;
Expand Down Expand Up @@ -123,7 +123,7 @@ export const layoutAssetGraph = (graphData: GraphData): AssetGraphLayout => {

// Add all the link nodes to the graph
Object.keys(linksToAssetsOutsideGraphedSet).forEach((id) => {
g.setNode(id, getAssetLinkDimensions(id));
g.setNode(id, getAssetLinkDimensions());
});

dagre.layout(g);
Expand Down Expand Up @@ -199,9 +199,10 @@ export const layoutAssetGraph = (graphData: GraphData): AssetGraphLayout => {
};
};

export const getAssetLinkDimensions = (id: string) => {
const path = JSON.parse(id);
return {width: displayNameForAssetKey({path}).length * 8 + 40, height: 40};
export const ASSET_LINK_NAME_MAX_LENGTH = 10;

export const getAssetLinkDimensions = () => {
return {width: 106, height: 40};
};

export const padBounds = (a: IBounds, padding: {x: number; top: number; bottom: number}) => {
Expand All @@ -221,13 +222,7 @@ export const extendBounds = (a: IBounds, b: IBounds) => {
return {x: xmin, y: ymin, width: xmax - xmin, height: ymax - ymin};
};

export const ASSET_NODE_ICON_WIDTH = 20;
export const ASSET_NODE_NAME_MAX_LENGTH = 32;
const DISPLAY_NAME_PX_PER_CHAR = 8.0;

export const assetNameMaxlengthForWidth = (width: number) => {
return (width - ASSET_NODE_ICON_WIDTH) / DISPLAY_NAME_PX_PER_CHAR;
};
export const ASSET_NODE_NAME_MAX_LENGTH = 28;

export const getAssetNodeDimensions = (def: {
assetKey: {path: string[]};
Expand All @@ -238,12 +233,7 @@ export const getAssetNodeDimensions = (def: {
description?: string | null;
computeKind: string | null;
}) => {
const displayName = def.assetKey.path[def.assetKey.path.length - 1];
const width =
Math.max(
230,
Math.min(ASSET_NODE_NAME_MAX_LENGTH, displayName.length) * DISPLAY_NAME_PX_PER_CHAR,
) + ASSET_NODE_ICON_WIDTH;
const width = 255;

if (def.isSource && !def.isObservable) {
return {width, height: 40};
Expand Down

0 comments on commit d62bd97

Please sign in to comment.