Skip to content

Commit

Permalink
[dagit] Use last path component of linked assets on the asset graph +…
Browse files Browse the repository at this point in the history
… add tooltips (#12590)

This should resolve #12028

### Summary & Motivation

- Asset links now have tooltips on hover

- Asset links now show only the last path component of the asset key,
which matches the behavior of full-blown asset nodes. Given {key: ['a',
'b']} the graph just shows "b". This should make the truncation more
acceptable.

- We previously showed "abcde..." instead of "abc...gi" if there were 6
or less characters of space available. We now do this for 10 or less
characters so the asset links do prefix truncation nicely (and in
general, it seemed like "abc...gi" was not great)

This achieves the behavior Sandy mentioned in #12028.

### How I Tested These Changes

- Asset links now have storybooks!

![image](https://user-images.githubusercontent.com/1037212/221935839-382cadd3-c842-4550-9be4-44c855e52854.png)

Co-authored-by: bengotow <bgotow@elementl.com>
  • Loading branch information
bengotow and bengotow committed Feb 28, 2023
1 parent bc130b8 commit 14337e8
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 14 deletions.
2 changes: 1 addition & 1 deletion js_modules/dagit/packages/core/src/app/Util.tsx
Expand Up @@ -24,7 +24,7 @@ export const withMiddleTruncation = (text: string, options: {maxLength: number})
// No truncation is necessary
return text;
}
if (options.maxLength <= 6) {
if (options.maxLength <= 10) {
// Middle truncation to this few characters (eg: abc…ef) is kind of silly
// and just using abcde… looks better.
return text.substring(0, options.maxLength - 1) + '…';
Expand Down
Expand Up @@ -5,6 +5,7 @@ import {KNOWN_TAGS} from '../graph/OpTags';

import {AssetNode, AssetNodeMinimal} from './AssetNode';
import * as Mocks from './AssetNode.mocks';
import {AssetNodeLink} from './ForeignNode';
import {getAssetNodeDimensions} from './layout';

// eslint-disable-next-line import/no-default-export
Expand Down Expand Up @@ -58,6 +59,15 @@ export const LiveStates = () => {
return;
};

export const Links = () => {
return (
<Box flex={{direction: 'column', gap: 0, alignItems: 'flex-start'}}>
<AssetNodeLink assetKey={{path: ['short_name']}} />
<AssetNodeLink assetKey={{path: ['multicomponent', 'key', 'path']}} />
<AssetNodeLink assetKey={{path: ['very_long_asset_in_another_graph']}} />
</Box>
);
};
export const PartnerTags = () => {
const caseWithComputeKind = (computeKind: string) => {
const def = {...Mocks.AssetNodeFragmentBasic, computeKind};
Expand Down
Expand Up @@ -8,7 +8,6 @@ import {
AssetNodeScenariosPartitioned,
AssetNodeScenariosSource,
} from './AssetNode.mocks';
import {displayNameForAssetKey} from './Utils';

const Scenarios = [
...AssetNodeScenariosBase,
Expand All @@ -30,7 +29,8 @@ describe('AssetNode', () => {
);

await waitFor(() => {
const displayName = displayNameForAssetKey(scenario.definition.assetKey);
const assetKey = scenario.definition.assetKey;
const displayName = assetKey.path[assetKey.path.length - 1];
expect(screen.getByText(displayName)).toBeVisible();
for (const text of scenario.expectedText) {
expect(screen.getByText(new RegExp(text))).toBeVisible();
Expand Down
25 changes: 14 additions & 11 deletions js_modules/dagit/packages/core/src/asset-graph/ForeignNode.tsx
Expand Up @@ -4,27 +4,30 @@ 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">
{withMiddleTruncation(displayNameForAssetKey(assetKey), {
maxLength: ASSET_LINK_NAME_MAX_LENGTH,
})}
</span>
</AssetNodeLinkContainer>
));
}> = React.memo(({assetKey}) => {
const label = assetKey.path[assetKey.path.length - 1];
return (
<AssetNodeLinkContainer>
<Icon name="open_in_new" color={Colors.Link} />
<span className="label" title={label}>
{withMiddleTruncation(label, {
maxLength: ASSET_LINK_NAME_MAX_LENGTH,
})}
</span>
</AssetNodeLinkContainer>
);
});

const AssetNodeLinkContainer = styled.div`
display: flex;
padding: 4px 8px 6px;
line-height: 30px;
font-family: ${FontFamily.monospace};
justify-content: center;
color: ${Colors.Link};
align-items: center;
font-weight: 600;
Expand Down

0 comments on commit 14337e8

Please sign in to comment.