Skip to content

Commit

Permalink
[ui] Fix vertical centering of arrows on the asset graph, asset name …
Browse files Browse the repository at this point in the history
…wrapping (#25974)

## Summary & Motivation

This fixes a couple specific issues with the asset graph and updates our
AssetNode stories:

- Lower the truncation threshold for asset names to prevent wrapping. I
think the previous value 38 must have been for a smaller font size, or
for a previous asset node width. I verified the current font size (14px)
is what is specified in Josh's latest designs and added a story for this
case.

<img width="354" alt="image"
src="https://github.com/user-attachments/assets/56d66c50-564a-4b18-a927-a3cb3d87664b">


- Update the `getAssetNodeDimensions` function to reflect what is
actually rendered and ensure the `AssetNodeBox` is always centered in
the layout box so that horizontal edges hit the middle of the box. To
make this easier I updated the storybooks to show the overall box in a
gray color and draw a midline where edges will arrive. I think it'd been
a while since we updated this and in a bunch of cases there was too much
padding at the bottom of the box, which we were compensating for by
giving asset groups a negative margin.

## How I Tested These Changes

<img width="1840" alt="image"
src="https://github.com/user-attachments/assets/32cfa1cd-4452-4733-ac8b-dbe490183cab">

- A few new storybook cases covering kind tags and combinations of
checks + partitions + change reasons.

- app-proxy testing against elementl prod's global graph, both in
horizontal and vertical mode and looking at maximally-tall assets at the
bottom of asset groups:

<img width="863" alt="image"
src="https://github.com/user-attachments/assets/7efc4ac8-37fd-4ee8-809e-0b3d7af94065">

---------

Co-authored-by: bengotow <bgotow@elementl.com>
  • Loading branch information
bengotow and bengotow authored Nov 19, 2024
1 parent 5a4f808 commit 2616e04
Show file tree
Hide file tree
Showing 6 changed files with 127 additions and 56 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {AssetNodeMenuProps, useAssetNodeMenu} from './AssetNodeMenu';
import {buildAssetNodeStatusContent} from './AssetNodeStatusContent';
import {ContextMenuWrapper} from './ContextMenuWrapper';
import {LiveDataForNode} from './Utils';
import {ASSET_NODE_NAME_MAX_LENGTH} from './layout';
import {
ASSET_NODE_NAME_MAX_LENGTH,
ASSET_NODE_STATUS_ROW_HEIGHT,
ASSET_NODE_TAGS_HEIGHT,
} from './layout';
import {gql} from '../apollo-client';
import {AssetNodeFragment} from './types/AssetNode.types';
import {withMiddleTruncation} from '../app/Util';
Expand All @@ -30,19 +34,23 @@ interface Props {

export const AssetNode = React.memo(({definition, selected, kindFilter}: Props) => {
const {liveData} = useAssetLiveData(definition.assetKey);
const hasChecks = (liveData?.assetChecks || []).length > 0;

const marginTopForCenteringNode = !hasChecks ? ASSET_NODE_STATUS_ROW_HEIGHT / 2 : 0;

return (
<AssetInsetForHoverEffect>
<Box
flex={{direction: 'row', justifyContent: 'space-between', alignItems: 'center'}}
style={{minHeight: 24}}
>
<StaleReasonsTag liveData={liveData} assetKey={definition.assetKey} />
<ChangedReasonsTag
changedReasons={definition.changedReasons}
assetKey={definition.assetKey}
/>
</Box>
<AssetNodeContainer $selected={selected}>
<Box
flex={{direction: 'row', justifyContent: 'space-between', alignItems: 'center'}}
style={{minHeight: ASSET_NODE_TAGS_HEIGHT, marginTop: marginTopForCenteringNode}}
>
<StaleReasonsTag liveData={liveData} assetKey={definition.assetKey} />
<ChangedReasonsTag
changedReasons={definition.changedReasons}
assetKey={definition.assetKey}
/>
</Box>
<AssetNodeBox $selected={selected} $isMaterializable={definition.isMaterializable}>
<AssetNameRow definition={definition} />
<Box style={{padding: '6px 8px'}} flex={{direction: 'column', gap: 4}} border="top">
Expand All @@ -59,16 +67,17 @@ export const AssetNode = React.memo(({definition, selected, kindFilter}: Props)
</Box>

<AssetNodeStatusRow definition={definition} liveData={liveData} />
{(liveData?.assetChecks || []).length > 0 && (
<AssetNodeChecksRow definition={definition} liveData={liveData} />
)}
{hasChecks && <AssetNodeChecksRow definition={definition} liveData={liveData} />}
</AssetNodeBox>
<Box flex={{direction: 'row-reverse', gap: 8}}>
<Box
style={{minHeight: ASSET_NODE_TAGS_HEIGHT}}
flex={{alignItems: 'center', direction: 'row-reverse', gap: 8}}
>
{definition.kinds.map((kind) => (
<AssetKind
key={kind}
kind={kind}
style={{position: 'relative', paddingTop: 7, margin: 0}}
style={{position: 'relative', margin: 0}}
currentPageFilter={kindFilter}
/>
))}
Expand Down Expand Up @@ -272,7 +281,7 @@ export const ASSET_NODE_FRAGMENT = gql`
`;

export const AssetInsetForHoverEffect = styled.div`
padding: 10px 4px 2px 4px;
padding: 2px 4px 2px 4px;
height: 100%;
& *:focus {
Expand All @@ -283,7 +292,7 @@ export const AssetInsetForHoverEffect = styled.div`
export const AssetNodeContainer = styled.div<{$selected: boolean}>`
user-select: none;
cursor: pointer;
padding: 6px;
padding: 0 6px;
overflow: clip;
`;

Expand All @@ -307,6 +316,7 @@ export const AssetNodeBox = styled.div<{
background: ${Colors.backgroundDefault()};
border-radius: 10px;
position: relative;
margin: 6px 0;
transition: all 150ms linear;
&:hover {
${(p) => !p.$selected && `border: 2px solid ${Colors.lineageNodeBorderHover()};`};
Expand All @@ -322,6 +332,7 @@ export const AssetNodeBox = styled.div<{
const NameCSS: CSSObject = {
padding: '3px 0 3px 6px',
color: Colors.textDefault(),
fontSize: 14,
fontFamily: FontFamily.monospace,
fontWeight: 600,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,7 @@ export const AssetNodeFragmentBasic: AssetNodeFragment = buildAssetNode({
jobNames: ['job1'],
opNames: ['asset1'],
opVersion: '1',
changedReasons: [
ChangeReason.NEW,
ChangeReason.CODE_VERSION,
ChangeReason.DEPENDENCIES,
ChangeReason.PARTITIONS_DEFINITION,
ChangeReason.TAGS,
ChangeReason.METADATA,
ChangeReason.REMOVED,
],
changedReasons: [],
});

export const AssetNodeFragmentSource = buildAssetNode({
Expand All @@ -88,13 +80,28 @@ export const AssetNodeFragmentSource = buildAssetNode({
});

export const AssetNodeFragmentSourceOverdue = buildAssetNode({
...AssetNodeFragmentBasic,
isMaterializable: false,
isObservable: false,
freshnessInfo: buildAssetFreshnessInfo({
currentMinutesLate: 12,
}),
});

export const AssetNodeFragmentChangedInBranch = buildAssetNode({
...AssetNodeFragmentBasic,
kinds: ['sql'],
changedReasons: [
ChangeReason.NEW,
ChangeReason.CODE_VERSION,
ChangeReason.DEPENDENCIES,
ChangeReason.PARTITIONS_DEFINITION,
ChangeReason.TAGS,
ChangeReason.METADATA,
ChangeReason.REMOVED,
],
});

export const AssetNodeFragmentPartitioned: AssetNodeFragment = buildAssetNode({
...AssetNodeFragmentBasic,
assetKey: buildAssetKey({path: ['asset_partioned']}),
Expand Down Expand Up @@ -789,6 +796,24 @@ export const AssetNodeScenariosBase = [
definition: AssetNodeFragmentBasic,
expectedText: ['Materialized', 'Checks'],
},
{
title: 'Changed in Branch',
liveData: LiveDataForNodeMaterializedWithChecks,
definition: AssetNodeFragmentChangedInBranch,
expectedText: ['New in branch'],
},
{
title: 'Very long key',
liveData: {
...LiveDataForNodeMaterialized,
stepKey: 'very_long_asset_which_was_totally_reasonable_at_the_time',
},
definition: {
...AssetNodeFragmentBasic,
assetKey: buildAssetKey({path: ['very_long_asset_which_was_totally_reasonable_at_the_time']}),
},
expectedText: [],
},
];

export const AssetNodeScenariosSource = [
Expand All @@ -801,10 +826,11 @@ export const AssetNodeScenariosSource = [

{
title: 'Source Asset - Not Observable',
liveData: undefined,
liveData: LiveDataForNodeNeverMaterialized,
definition: {
...AssetNodeFragmentSource,
isObservable: false,
isExecutable: false,
id: '["source_asset_no"]',
assetKey: buildAssetKey({path: ['source_asset_no']}),
},
Expand All @@ -813,10 +839,11 @@ export const AssetNodeScenariosSource = [

{
title: 'Source Asset - Not Observable, No Description',
liveData: undefined,
liveData: LiveDataForNodeNeverMaterialized,
definition: {
...AssetNodeFragmentSource,
isObservable: false,
isExecutable: false,
description: null,
id: '["source_asset_nono"]',
assetKey: buildAssetKey({path: ['source_asset_nono']}),
Expand Down Expand Up @@ -902,12 +929,27 @@ export const AssetNodeScenariosPartitioned = [
expectedText: ['1,500 partitions', 'All'],
},

{
title: 'Partitioned Asset - Checks and Tags',
liveData: {
...LiveDataForNodePartitionedFresh,
assetChecks: LiveDataForNodeMaterializedWithChecks.assetChecks,
},
definition: {
...AssetNodeFragmentPartitioned,
changedReasons: [ChangeReason.NEW],
kinds: ['ipynb'],
},
expectedText: ['1,500 partitions', 'All', 'Checks'],
},

{
title: 'Partitioned Asset - Last Run Failed',
liveData: LiveDataForNodePartitionedLatestRunFailed,
definition: AssetNodeFragmentPartitioned,
expectedText: ['4', '999+', '1,500 partitions'],
},

{
title: 'Partitioned Asset - Live Data Loading',
liveData: undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,24 +56,32 @@ export const LiveStates = () => {
<>
<SetCacheEntry />
<Box flex={{direction: 'column', gap: 8, alignItems: 'flex-start'}}>
<code style={{marginTop: 20}}>
<strong>{scenario.title}</strong>
</code>
<div
style={{
position: 'relative',
width: dimensions.width,
height: dimensions.height,
background: `linear-gradient(to bottom, rgba(100,100,100,0.15) 49%, rgba(100,100,100,1) 50%, rgba(100,100,100,0.15) 51%)`,
overflowY: 'hidden',
}}
>
<AssetNode definition={definitionCopy} selected={false} />
</div>
<div style={{position: 'relative', width: dimensions.width, height: 104}}>
<div style={{position: 'absolute', width: dimensions.width}}>
<div
style={{
position: 'relative',
width: dimensions.width,
background: `rgba(100,100,100,0.15)`,
height: 90,
}}
>
<div style={{position: 'absolute', width: dimensions.width, transform: 'scale(0.4)'}}>
<AssetNodeMinimal definition={definitionCopy} selected={false} height={82} />
</div>
</div>
<code>
<strong>{scenario.title}</strong>
</code>
</Box>
</>
);
Expand All @@ -82,13 +90,15 @@ export const LiveStates = () => {
return (
<MockedProvider>
<AssetLiveDataProvider>
<h2>Base Assets</h2>
<Box flex={{gap: 20, wrap: 'wrap', alignItems: 'flex-start'}}>
{Mocks.AssetNodeScenariosBase.map(caseWithLiveData)}
</Box>

<h2>Source Assets</h2>
<Box flex={{gap: 20, wrap: 'wrap', alignItems: 'flex-start'}}>
{Mocks.AssetNodeScenariosSource.map(caseWithLiveData)}
</Box>
<h2>Partitioned Assets</h2>
<Box flex={{gap: 20, wrap: 'wrap', alignItems: 'flex-start'}}>
{Mocks.AssetNodeScenariosPartitioned.map(caseWithLiveData)}
</Box>
Expand All @@ -107,9 +117,10 @@ export const Links = () => {
</Box>
);
};

export const PartnerTags = () => {
const caseWithComputeKind = (computeKind: string) => {
const def = {...Mocks.AssetNodeFragmentBasic, computeKind};
const def = {...Mocks.AssetNodeFragmentBasic, kinds: [computeKind]};
const liveData = Mocks.LiveDataForNodeMaterialized;

function SetCacheEntry() {
Expand Down Expand Up @@ -142,6 +153,7 @@ export const PartnerTags = () => {
width: 280,
height: dimensions.height,
overflowY: 'hidden',
background: `linear-gradient(to bottom, transparent 49%, gray 50%, transparent 51%)`,
}}
>
<AssetNode definition={def} selected={false} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {MockedProvider} from '@apollo/client/testing';
import {render, screen, waitFor} from '@testing-library/react';
import {MemoryRouter} from 'react-router-dom';

import {withMiddleTruncation} from '../../app/Util';
import {AssetBaseData} from '../../asset-data/AssetBaseDataProvider';
import {AssetLiveDataProvider} from '../../asset-data/AssetLiveDataProvider';
import {AssetStaleStatusData} from '../../asset-data/AssetStaleStatusDataProvider';
Expand All @@ -13,6 +14,7 @@ import {
AssetNodeScenariosPartitioned,
AssetNodeScenariosSource,
} from '../__fixtures__/AssetNode.fixtures';
import {ASSET_NODE_NAME_MAX_LENGTH} from '../layout';

const Scenarios = [
...AssetNodeScenariosBase,
Expand Down Expand Up @@ -66,7 +68,13 @@ describe('AssetNode', () => {
await waitFor(() => {
const assetKey = definitionCopy.assetKey;
const displayName = assetKey.path[assetKey.path.length - 1]!;
expect(screen.getByText(displayName)).toBeVisible();
expect(
screen.getByText(
withMiddleTruncation(displayName, {
maxLength: ASSET_NODE_NAME_MAX_LENGTH,
}),
),
).toBeVisible();
for (const text of scenario.expectedText) {
expect(screen.getByText(new RegExp(text))).toBeVisible();
}
Expand Down
34 changes: 16 additions & 18 deletions js_modules/dagster-ui/packages/ui-core/src/asset-graph/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export const Config = {
nodesep: -10,
nodeHeight: 'auto',
groupPaddingTop: 65,
groupPaddingBottom: -15,
groupPaddingBottom: -4,
groupRendering: 'if-varied',
clusterpaddingtop: 100,
},
Expand All @@ -86,7 +86,7 @@ export const Config = {
edgesep: 10,
nodeHeight: 'auto',
groupPaddingTop: 55,
groupPaddingBottom: -5,
groupPaddingBottom: -4,
groupRendering: 'if-varied',
},
};
Expand Down Expand Up @@ -338,7 +338,10 @@ export const extendBounds = (a: IBounds, b: IBounds) => {
};

export const ASSET_NODE_WIDTH = 320;
export const ASSET_NODE_NAME_MAX_LENGTH = 38;
export const ASSET_NODE_TAGS_HEIGHT = 28;
export const ASSET_NODE_STATUS_ROW_HEIGHT = 25;

export const ASSET_NODE_NAME_MAX_LENGTH = 31;

export const getAssetNodeDimensions = (def: {
assetKey: {path: string[]};
Expand All @@ -351,24 +354,19 @@ export const getAssetNodeDimensions = (def: {
computeKind: string | null;
changedReasons?: ChangeReason[];
}) => {
const width = ASSET_NODE_WIDTH;
let height = 0;

let height = 106; // top tags area + name + description
height += ASSET_NODE_TAGS_HEIGHT; // top tags

if (!def.isMaterializable && def.isObservable) {
height += 30; // status row
} else {
height += 28; // status row
height += 28; // checks row
if (def.isPartitioned) {
height += 52;
}
}
if (def.changedReasons?.length) {
height += 30;
height += 76; // box padding + border + name + description

if (def.isPartitioned && def.isMaterializable) {
height += ASSET_NODE_STATUS_ROW_HEIGHT;
}

height += 36; // tags beneath
height += ASSET_NODE_STATUS_ROW_HEIGHT; // status row
height += ASSET_NODE_STATUS_ROW_HEIGHT; // checks row
height += ASSET_NODE_TAGS_HEIGHT; // bottom tags

return {width, height};
return {width: ASSET_NODE_WIDTH, height};
};
Loading

1 comment on commit 2616e04

@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-o2zzyaliu-elementl.vercel.app

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

Please sign in to comment.