From 5b628dbf9ab4af241e9cf9ba04fd99d7f32c48fb Mon Sep 17 00:00:00 2001 From: Ben Gotow Date: Tue, 10 Dec 2024 21:00:50 -0600 Subject: [PATCH] [ui] For runs targeting assets, show >1 asset in the Target column (#26122) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve had reports that users who launch small N numbers of assets together find the new run feed UI difficult to use, since it shows either “asset name” or “2 assets” in the Target column. The "2 assets" form makes it difficult to scan for a run you have in mind: image This PR: - Makes the target column a bit wider - Shows as many asset / check tags as will fit in the available space, and then “X more” The implementation of this uses a new hook `useAdjustChildVisibilityToFill`. The idea is that your component renders some reasonable max number of tags (10) and a more tag, and the hook uses a resize observer + layout effect to show/hide the tags to fit the available space. I tried doing this using React state, but it looks bad because you can see it adding / removing tags, especially as you resize the viewport. I think the other approach would be to write a tag “measure” function, or otherwise repeatedly render + size them offscreen, but that’d still force layouts with the added cost of a React.render (the tags are not identical react components - the “4 more” tag is slightly different - so I think the offscreen approach would need to actually render the tag...) The text + size of the "more" tag also changes based on the number of tags displayed. Sidenote: There’s a bunch of cruft here because the “Target” column components all have to support a “tags” rendering and a “plain” rendering. This is going away soon when we remove the FF allowing users to revert to the old runs page, and I think it’ll clean up this code! ## How I Tested These Changes I added a storybook that makes it easy to test what this looks like in a bunch of scenarios and verify it works nicely. https://github.com/user-attachments/assets/8dc2afdb-e15c-4fab-a139-eb5698428bf1 Before: image image After: image image Related: https://linear.app/dagster-labs/issue/FE-702/show-more-than-one-asset-tag-on-the-new-runs-feed-page Co-authored-by: bengotow --- .../backfill/BackfillOpJobPartitionsTable.tsx | 2 +- .../src/instance/backfill/BackfillRow.tsx | 60 ++- .../src/pipelines/PipelineReference.tsx | 17 +- .../ui-core/src/runs/AssetTagCollections.tsx | 366 ++++++++++++------ .../ui-core/src/runs/RunAssetTags.tsx | 14 +- .../packages/ui-core/src/runs/RunRow.tsx | 2 +- .../ui-core/src/runs/RunTargetLink.tsx | 36 +- .../packages/ui-core/src/runs/RunsFeedRow.tsx | 25 +- .../AssetKeyTagCollection.stories.tsx | 107 +++++ 9 files changed, 452 insertions(+), 177 deletions(-) create mode 100644 js_modules/dagster-ui/packages/ui-core/src/runs/__stories__/AssetKeyTagCollection.stories.tsx diff --git a/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillOpJobPartitionsTable.tsx b/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillOpJobPartitionsTable.tsx index 291a5465903b0..5e6be3050640a 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillOpJobPartitionsTable.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/instance/backfill/BackfillOpJobPartitionsTable.tsx @@ -60,7 +60,7 @@ export const BackfillOpJobPartitionsTable = ({ - + {showBackfillTarget ? ( - + ) : null} @@ -146,9 +146,11 @@ export const BackfillRowContent = ({ export const BackfillTarget = ({ backfill, repoAddress, + useTags, }: { backfill: Pick; repoAddress: RepoAddress | null; + useTags: boolean; }) => { const repo = useRepository(repoAddress); const {assetSelection, partitionSet, partitionSetName} = backfill; @@ -160,22 +162,30 @@ export const BackfillTarget = ({ return null; } if (partitionSet && repo) { + const link = workspacePipelinePath({ + repoName: partitionSet.repositoryOrigin.repositoryName, + repoLocation: partitionSet.repositoryOrigin.repositoryLocationName, + pipelineName: partitionSet.pipelineName, + isJob: isThisThingAJob(repo, partitionSet.pipelineName), + path: `/partitions?partitionSet=${encodeURIComponent(partitionSet.name)}`, + }); + if (useTags) { + return ( + + {partitionSet.name} + + ); + } return ( - + {partitionSet.name} ); } if (partitionSetName) { + if (useTags) { + return {partitionSetName}; + } return {partitionSetName}; } return null; @@ -193,10 +203,28 @@ export const BackfillTarget = ({ const buildPipelineOrAssets = () => { if (assetSelection?.length) { - return ; + return ( + + ); } if (partitionSet && repo) { - return ( + return useTags ? ( + + ) : ( + {buildHeader()} {(pipelineOrAssets || repoLink) && ( - {repoLink} + {repoLink && useTags ? {repoLink} : repoLink} {pipelineOrAssets} )} diff --git a/js_modules/dagster-ui/packages/ui-core/src/pipelines/PipelineReference.tsx b/js_modules/dagster-ui/packages/ui-core/src/pipelines/PipelineReference.tsx index 0a6def3211b08..66ee623b3c9e5 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/pipelines/PipelineReference.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/pipelines/PipelineReference.tsx @@ -1,6 +1,5 @@ import {Box, Colors, Icon, Tag} from '@dagster-io/ui-components'; import {Link} from 'react-router-dom'; -import styled from 'styled-components'; import {PipelineSnapshotLink} from './PipelinePathUtils'; import {RepoAddress} from '../workspace/types'; @@ -54,7 +53,7 @@ export const PipelineReference = ({ return ( {showIcon && ( - + )} @@ -71,16 +70,8 @@ export const PipelineReference = ({ export const PipelineTag = (props: Props) => { return ( - - - - - + + + ); }; - -const PipelineTagWrap = styled.span` - span { - line-height: 0; - } -`; diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/AssetTagCollections.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/AssetTagCollections.tsx index 394ca93f0db74..ca9220bf5177f 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/AssetTagCollections.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/AssetTagCollections.tsx @@ -9,10 +9,11 @@ import { MiddleTruncate, Tag, } from '@dagster-io/ui-components'; +import useResizeObserver from '@react-hook/resize-observer'; import * as React from 'react'; import {Link} from 'react-router-dom'; -import {displayNameForAssetKey} from '../asset-graph/Utils'; +import {displayNameForAssetKey, tokenForAssetKey} from '../asset-graph/Utils'; import { assetDetailsPathForAssetCheck, assetDetailsPathForKey, @@ -72,84 +73,182 @@ interface AssetKeyTagCollectionProps { assetKeys: AssetKey[] | null; dialogTitle?: string; useTags?: boolean; + maxRows?: number; +} + +/** This hook returns a containerRef and a moreLabelRef. It expects you to populate + * containerRef with children, with the last child being a "More" button. When your + * container renders, children will be hidden until the container is not overflowing + * its maxHeight. + * + * This hook waits for an animation frame but forces layout so you can't see it "trying" + * different numbers of tags. To avoid seeing tags wrap while waiting for an animation + * frame, place an `overflow: hidden` on the container. + */ +export function useAdjustChildVisibilityToFill(moreLabelFn: (count: number) => string | null) { + const containerRef = React.createRef(); + const moreLabelRef = React.createRef(); + const evaluatingRef = React.useRef(false); + + const evaluate = React.useCallback(() => { + const container = containerRef.current; + if (!container) { + return; + } + + const children = Array.from(container.children) as HTMLElement[]; + if (children.length < 2) { + return; // single item or no items, no need for fanciness + } + + const tagsEls = children.slice(0, -1); + const moreEl = children.pop()!; + + const apply = () => { + const moreLabel = moreLabelFn(count); + if (moreLabelRef.current && moreLabelRef.current.textContent !== moreLabel) { + moreLabelRef.current.textContent = moreLabel; + } + moreEl.style.display = moreLabel !== null ? 'initial' : 'none'; + tagsEls.forEach((c, idx) => (c.style.display = idx < count ? 'initial' : 'none')); + }; + + evaluatingRef.current = true; + + let count = 0; + apply(); + + while (true) { + if (container.scrollHeight > container.offsetHeight && count > 0) { + count--; + apply(); + break; + } else if (count < tagsEls.length) { + count++; + apply(); + } else { + break; + } + } + + evaluatingRef.current = false; + }, [moreLabelFn, moreLabelRef, containerRef]); + + useResizeObserver(containerRef, () => { + if (!evaluatingRef.current) { + evaluate(); + } + }); + + React.useEffect(() => { + window.requestAnimationFrame(evaluate); + }, [evaluate]); + + return {containerRef, moreLabelRef}; } export const AssetKeyTagCollection = React.memo((props: AssetKeyTagCollectionProps) => { - const {assetKeys, useTags, dialogTitle = 'Assets in run'} = props; + const {assetKeys, useTags, maxRows, dialogTitle = 'Assets in run'} = props; const {setShowMore, dialog} = useShowMoreDialog(dialogTitle, assetKeys, renderItemAssetKey); + const count = assetKeys?.length ?? 0; + const rendered = maxRows ? 10 : count === 1 ? 1 : 0; + const moreLabelFn = React.useCallback( + (displayed: number) => + displayed === 0 + ? `${numberFormatter.format(count)} assets` + : count - displayed > 0 + ? `${numberFormatter.format(count - displayed)} more` + : null, + [count], + ); + + const {containerRef, moreLabelRef} = useAdjustChildVisibilityToFill(moreLabelFn); + if (!assetKeys || !assetKeys.length) { return null; } - if (assetKeys.length === 1) { - // Outer span ensures the popover target is in the right place if the - // parent is a flexbox. - const assetKey = assetKeys[0]!; - return ( - - {useTags ? ( - - - - ) : ( - - - - - - - )} - - ); - } - return ( - - setShowMore(true), - }, - { - label: 'View lineage', - to: globalAssetGraphPathForAssetsAndDescendants(assetKeys), - }, - ]} - > - {useTags ? ( - - {numberFormatter.format(assetKeys.length)} assets - - ) : ( - setShowMore(true)} underline="hover"> - - - {`${numberFormatter.format(assetKeys.length)} assets`} - - - )} - - {dialog} - + + {assetKeys.slice(0, rendered).map((assetKey) => ( + // Outer span ensures the popover target is in the right place if the + // parent is a flexbox. + + {useTags ? ( + + + + ) : ( + + + + + + + )} + + ))} + {rendered !== 1 && ( + + setShowMore(true), + }, + { + label: 'View lineage', + to: globalAssetGraphPathForAssetsAndDescendants(assetKeys), + }, + ]} + > + {useTags ? ( + + {moreLabelFn(0)} + + ) : ( + setShowMore(true)} underline="hover"> + + + {moreLabelFn(0)} + + + )} + + {dialog} + + )} + ); }); @@ -159,67 +258,92 @@ interface AssetCheckTagCollectionProps { assetChecks: Check[] | null; dialogTitle?: string; useTags?: boolean; + maxRows?: number; } export const AssetCheckTagCollection = React.memo((props: AssetCheckTagCollectionProps) => { - const {assetChecks, useTags, dialogTitle = 'Asset checks in run'} = props; + const {assetChecks, maxRows, useTags, dialogTitle = 'Asset checks in run'} = props; const {setShowMore, dialog} = useShowMoreDialog(dialogTitle, assetChecks, renderItemAssetCheck); + const count = assetChecks?.length ?? 0; + const rendered = maxRows ? 10 : count === 1 ? 1 : 0; + const moreLabelFn = React.useCallback( + (displayed: number) => + displayed === 0 + ? `${numberFormatter.format(count)} checks` + : count - displayed > 0 + ? `${numberFormatter.format(count - displayed)} more` + : null, + [count], + ); + + const {containerRef, moreLabelRef} = useAdjustChildVisibilityToFill(moreLabelFn); + if (!assetChecks || !assetChecks.length) { return null; } - if (assetChecks.length === 1) { - // Outer span ensures the popover target is in the right place if the - // parent is a flexbox. - const check = assetChecks[0]!; - return ( - - {useTags ? ( - - - - ) : ( - - - - - - - )} - - ); - } - return ( - <> - setShowMore(true), - }, - ]} - > - {useTags ? ( - - {numberFormatter.format(assetChecks.length)} asset checks - - ) : ( - setShowMore(true)} underline="hover"> - - - {`${numberFormatter.format(assetChecks.length)} asset checks`} - - - )} - - {dialog} - + + {assetChecks.slice(0, rendered).map((check) => ( + + {useTags ? ( + + + + ) : ( + + + + + + + )} + + ))} + {rendered !== 1 && ( + + setShowMore(true), + }, + ]} + > + {useTags ? ( + + {moreLabelFn(0)} + + ) : ( + setShowMore(true)} underline="hover"> + + + {moreLabelFn(0)} + + + )} + + {dialog} + + )} + ); }); diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunAssetTags.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunAssetTags.tsx index 561412a038af1..867d462940da6 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunAssetTags.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunAssetTags.tsx @@ -1,3 +1,5 @@ +import {useMemo} from 'react'; + import {AssetKeyTagCollection} from './AssetTagCollections'; import {assetKeysForRun} from './RunUtils'; import {gql, useQuery} from '../apollo-client'; @@ -13,13 +15,15 @@ export const RunAssetTags = (props: {run: RunFragment}) => { skip, fetchPolicy: 'no-cache', }); - const {data, loading} = queryResult; - if (loading || !data || data.pipelineRunOrError.__typename !== 'Run') { - return null; - } + const assetKeys = useMemo(() => { + const {data, loading} = queryResult; + if (loading || !data || data.pipelineRunOrError.__typename !== 'Run') { + return null; + } - const assetKeys = skip ? assetKeysForRun(run) : data.pipelineRunOrError.assets.map((a) => a.key); + return skip ? assetKeysForRun(run) : data.pipelineRunOrError.assets.map((a) => a.key); + }, [queryResult, run, skip]); return ; }; diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/RunRow.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/RunRow.tsx index a79f148bffad4..80c9c836f95c6 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/runs/RunRow.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/RunRow.tsx @@ -85,7 +85,7 @@ export const RunRow = ({ - + ; repoAddress: RepoAddress | null; + useTags: boolean; }) => { - return isHiddenAssetGroupJob(run.pipelineName) ? ( - - - - + const assetKeys = React.useMemo(() => { + return isHiddenAssetGroupJob(run.pipelineName) ? assetKeysForRun(run) : null; + }, [run]); + + if (assetKeys) { + return ( + + + + + ); + } + return useTags ? ( + ) : ( - - - {entry.__typename === 'Run' ? ( - - ) : ( - - )} - - + {entry.__typename === 'Run' ? ( + + ) : ( + + )} @@ -199,7 +196,7 @@ export const RunsFeedRow = ({ }; const TEMPLATE_COLUMNS = - '60px minmax(0, 2fr) minmax(0, 1.2fr) minmax(0, 1fr) 140px 170px 120px 132px'; + '60px minmax(0, 1.5fr) minmax(0, 1.2fr) minmax(0, 1fr) 140px 170px 120px 132px'; export const RunsFeedTableHeader = ({checkbox}: {checkbox: React.ReactNode}) => { return ( diff --git a/js_modules/dagster-ui/packages/ui-core/src/runs/__stories__/AssetKeyTagCollection.stories.tsx b/js_modules/dagster-ui/packages/ui-core/src/runs/__stories__/AssetKeyTagCollection.stories.tsx new file mode 100644 index 0000000000000..3e4784e58e43d --- /dev/null +++ b/js_modules/dagster-ui/packages/ui-core/src/runs/__stories__/AssetKeyTagCollection.stories.tsx @@ -0,0 +1,107 @@ +import {Box} from '@dagster-io/ui-components'; +import {Meta} from '@storybook/react'; +import faker from 'faker'; +import {MemoryRouter} from 'react-router-dom'; +import styled from 'styled-components'; + +import {AssetKeyTagCollection} from '../AssetTagCollections'; + +const makeKeys = (count: number) => { + return Array(count) + .fill(null) + .map((_) => ({path: [faker.random.word()]})); +}; + +// eslint-disable-next-line import/no-default-export +export default { + title: 'AssetKeyTagCollection', + component: AssetKeyTagCollection, +} as Meta; + +export const Scenarios = () => { + const single = makeKeys(1); + const two = makeKeys(2); + const many = makeKeys(20); + + const singleSuperLong = [{path: [faker.lorem.word(10).repeat(10)]}]; + const manySuperLong = Array(20) + .fill(null) + .map((_) => ({path: [faker.lorem.word(10).repeat(10)]})); + + return ( + + +
old style
+
tag style
+
tag + maxrows
+ + +
+ +
+
+ +
+
+ +
+ + +
+ +
+
+ +
+
+ +
+ + +
+ +
+
+ +
+
+ +
+ + +
+ +
+
+ +
+
+ +
+ + +
+ +
+
+ +
+
+ +
+ + + ); +}; + +const Row = styled(Box)` + display: grid; + grid-template-columns: 1fr 1fr 1fr; + min-height: 30px; + margin-bottom: 24px; + height: 100%; + + & > div { + min-width: 0; + } +`;