New asset catalog page #20210

clairelin135 · 2024-03-02T01:28:00Z

First stab at displaying the new asset catalog page.

Relies on the existing assetsOrError query to fetch asset definition info, and adds logic to group assets by code location / owner / etc. This will enable changes to this view, i.e. displaying a tooltip of the asset keys per group, if desired.

Tagging others on this PR to get thoughts so far on the existing implementation.

Next steps:

Add tests for this component
Unify styling for UserDisplay component across core and cloud. Currently they display differently.
Supporting asset search
Displaying a background for the search section

Outstanding questions:

How do we handle large numbers of assets on this page? Do we need to implement some way to minimize each section, or paginate?

github-actions · 2024-03-02T01:30:55Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-2qb86yz26-elementl.vercel.app
https://claire-asset-overview-page.components-storybook.dagster-docs.io

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

github-actions · 2024-03-02T01:31:19Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-99f7930l1-elementl.vercel.app
https://claire-asset-overview-page.core-storybook.dagster-docs.io

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

clairelin135 · 2024-03-04T18:06:29Z

[ui] Add tests for recently visited assets #20321
[ui] Enable displaying recently visited assets #20266
New asset catalog page #20210 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clairelin135 and the rest of your teammates on Graphite

js_modules/dagster-ui/packages/ui-components/src/components/BaseTag.tsx

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

hellendag

Nice! I've got some comments and requests in line. :)

hellendag · 2024-03-05T14:51:58Z

js_modules/dagster-ui/packages/ui-components/src/components/BaseTag.tsx

@@ -37,9 +38,15 @@ export const BaseTag = (props: Props) => {
    rightIcon,
    label,
    tooltipText,
+    fontSize = '12px',


Can you share a screenshot of where this change is needed? It's surprising to me that we would need to modify core components for this UI.

If it does need to be changed, can you separate the ui-components change into its own PR, with an added Storybook example to demonstrate the new value?

Without it, owners that don't exist as users display in a smaller font size:

Though perhaps we should not use a BaseTag for the OSS user display component and instead make the component match the internal implementation: https://github.com/dagster-io/internal/blob/87c74af6b49beef51c95b398cc0c1713b7795d42/dagster-cloud/js_modules/app-cloud/src/settings/audit-log/AuditLogUserDisplay.tsx#L29-L34

Regardless, will break this out into a separate diff.

Yeah, I think probably Tag (or BaseTag) aren't quite right for it, if all you really need is the subway dot and the team/user name. UserDisplayForPermissions might be close.

js_modules/dagster-ui/packages/ui-core/src/assets/AssetPageHeader.tsx

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

hellendag · 2024-03-05T14:58:32Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+  const assetCountByCodeLocation: Record<string, number> = {};
+
+  assets.forEach((asset) => {
+    if (asset.definition) {


Since it looks like you don't really need to run this forEach function if there's no asset.definition, perhaps use an assets.filter first?

hellendag · 2024-03-05T14:59:02Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+  repositoryLocationName: string;
+  repositoryName: string;


RepoAddress is nice for combining these values.

I did want to use RepoAddress here too, but we build a URL linking to the asset graph filtered view from this object that relies on these specific key names.

If desired, I can use RepoAddress here and then change the key names when we build the link

Ah yeah, I just mean that RepoAddress is nice for representing the combined strings in a single value to be passed around. Then when it needs to be stringified, we have the utilities to do that. Mostly I'd just want consistency, to having places where we are manually concatting or interpolating these values in a way that differs from how we represent RepoAddress strings elsewhere.

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

hellendag · 2024-03-05T15:10:34Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+            <SectionBody>
+              {Object.entries(assetCountBySection.countsByComputeKind).map(
+                ([label, count], idx) => (
+                  <CountForAssetType key={idx} assetsCount={count}>


Same as above.

hellendag · 2024-03-05T15:11:03Z

js_modules/dagster-ui/packages/ui-core/src/graph/OpTags.tsx

@@ -616,6 +616,25 @@ export const OpTags = React.memo(({tags, style, reduceColor, reduceText}: OpTags
  );
 });

+export const TagIcon = React.memo(({tag_label}: {tag_label: string}) => {


Always use camelCase for JS variable names.

hellendag · 2024-03-05T15:11:17Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+                ([label, count], idx) => (
+                  <CountForAssetType key={idx} assetsCount={count}>
+                    <Box flex={{direction: 'row', gap: 4, alignItems: 'center'}}>
+                      <TagIcon tag_label={label} />


See comment below re: JS var naming conventions.

hellendag · 2024-03-05T15:11:47Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+            <SectionHeader sectionName="Asset groups" />
+            <SectionBody>
+              {assetCountBySection.countPerAssetGroup.map(
+                (assetGroupCount: CountPerGroupName, idx) => (


You probably shouldn't have to annotate this type, TS should be able to infer it.

bengotow · 2024-03-06T00:10:28Z

Looks like Dish and Marco beat me to the PR reviews - this is looking awesome! 🙌

clairelin135 · 2024-03-06T00:11:09Z

Thanks @hellendag and @salazarm for all the feedback, super helpful to get a better understanding of best practices here.

I updated the PR per the feedback, I think it's ready for another look!

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

js_modules/dagster-ui/packages/ui-core/src/graph/OpTags.tsx

salazarm

This looks great!

salazarm

How will we get to this page? I didn't see any links to it in the other PR

hellendag

Sweet. Let's ship and iterate. Nice work!

hellendag · 2024-03-07T15:09:57Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+      <AssetPageHeader
+        assetKey={{path: currentPath}}
+        right={
+          <Box flex={{gap: 12, alignItems: 'center'}}>


With only one item in the flex container, the gap shouldn't be necessary here.

hellendag · 2024-03-07T15:11:09Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+      timeZone: timezone === 'Automatic' ? browserTimezone() : timezone,
+    }),
+  );
+  if (hour < 12) {


Should it be Good morning when it's like midnight or 2am? 🤔

hellendag · 2024-03-07T15:31:13Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+    new Date().toLocaleTimeString('en-US', {
+      hour: '2-digit',
+      hourCycle: 'h23',
+      timeZone: timezone === 'Automatic' ? browserTimezone() : timezone,
+    }),


No need to change anything here, but a couple notes:

For future reference, these formatters can be expensive to set up. It's often a good idea to create a (memoized or static) single formatter object for reuse and call format on it.

The approach of parsing out the number from the string looks good to me. Alternatively, if we chose not to care about the user's timezone setting, you could use the getHours() method on the Date object.

Thanks for the heads up here.

Which item exactly is the formatter? Is it the date object?

hellendag · 2024-03-07T15:33:17Z

js_modules/dagster-ui/packages/ui-core/src/app/ContentRoot.tsx

@@ -8,6 +8,7 @@ const WorkspaceRoot = lazy(() => import('../workspace/WorkspaceRoot'));
 const OverviewRoot = lazy(() => import('../overview/OverviewRoot'));
 const FallthroughRoot = lazy(() => import('./FallthroughRoot'));
 const AssetsCatalogRoot = lazy(() => import('../assets/AssetsCatalogRoot'));
+const AssetsOverview = lazy(() => import('../assets/AssetsOverview'));


Not used? Does this need a route added to the Switch?

Ah good catch, this will only be displayed on cloud so this line isn't needed

clairelin135 · 2024-03-07T18:28:57Z

@salazarm no links pointing to this page yet. I was thinking that we'd only swap it with the current assets table view once the search UI is out.

First stab at displaying the new asset catalog page. Relies on the existing `assetsOrError` query to fetch asset definition info, and adds logic to group assets by code location / owner / etc. This will enable changes to this view, i.e. displaying a tooltip of the asset keys per group, if desired. Tagging others on this PR to get thoughts so far on the existing implementation. Next steps: - Add tests for this component - Unify styling for `UserDisplay` component across core and cloud. Currently they display differently. - Supporting asset search - Displaying a background for the search section Outstanding questions: - How do we handle large numbers of assets on this page? Do we need to implement some way to minimize each section, or paginate? <img width="1457" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/de586d6b-8ee9-4f62-9891-b491e937f562">

clairelin135 force-pushed the claire/asset-overview-page branch from 88f4ede to 9989084 Compare March 4, 2024 18:06

clairelin135 requested review from salazarm, bengotow and hellendag March 4, 2024 18:18

salazarm reviewed Mar 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-components/src/components/BaseTag.tsx Outdated Show resolved Hide resolved

salazarm reviewed Mar 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx Show resolved Hide resolved

salazarm reviewed Mar 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx Outdated Show resolved Hide resolved

salazarm reviewed Mar 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx Outdated Show resolved Hide resolved

salazarm reviewed Mar 5, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx Outdated Show resolved Hide resolved

hellendag requested changes Mar 5, 2024

View reviewed changes

clairelin135 mentioned this pull request Mar 5, 2024

[ui] Enable displaying recently visited assets #20266

Merged

clairelin135 force-pushed the claire/asset-overview-page branch from 0b301e9 to ebfc8fc Compare March 5, 2024 23:21

clairelin135 requested review from hellendag and salazarm March 6, 2024 00:11

clairelin135 force-pushed the claire/asset-overview-page branch from ebfc8fc to 176d325 Compare March 6, 2024 00:50

salazarm reviewed Mar 6, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx Outdated Show resolved Hide resolved

clairelin135 force-pushed the claire/asset-overview-page branch from 176d325 to b29093f Compare March 6, 2024 18:23

salazarm reviewed Mar 6, 2024

View reviewed changes

js_modules/dagster-ui/packages/ui-core/src/graph/OpTags.tsx Outdated Show resolved Hide resolved

salazarm reviewed Mar 6, 2024

View reviewed changes

clairelin135 force-pushed the claire/asset-overview-page branch from b29093f to 826fa28 Compare March 6, 2024 21:50

clairelin135 mentioned this pull request Mar 7, 2024

[ui] Add tests for recently visited assets #20321

Merged

clairelin135 force-pushed the claire/asset-overview-page branch from 826fa28 to 9f8c949 Compare March 7, 2024 00:51

salazarm approved these changes Mar 7, 2024

View reviewed changes

hellendag approved these changes Mar 7, 2024

View reviewed changes

clairelin135 added 5 commits March 7, 2024 10:45

first stab at asset overview page

4d0450b

clean up

54aa785

update per pr feedback

f58afe8

fix scrolling

2b794f2

addressing feedback

67a2a67

clairelin135 force-pushed the claire/asset-overview-page branch from 8cb67cf to 67a2a67 Compare March 7, 2024 18:45

clairelin135 merged commit 258bc71 into master Mar 7, 2024
2 checks passed

clairelin135 deleted the claire/asset-overview-page branch March 7, 2024 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New asset catalog page #20210

New asset catalog page #20210

clairelin135 commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Mar 2, 2024 •

edited

Loading

clairelin135 commented Mar 4, 2024 •

edited

Loading

hellendag left a comment

hellendag Mar 5, 2024

clairelin135 Mar 5, 2024 •

edited

Loading

hellendag Mar 7, 2024

hellendag Mar 5, 2024

hellendag Mar 5, 2024

clairelin135 Mar 5, 2024 •

edited

Loading

hellendag Mar 7, 2024

hellendag Mar 5, 2024

hellendag Mar 5, 2024

hellendag Mar 5, 2024

hellendag Mar 5, 2024

bengotow commented Mar 6, 2024 •

edited

Loading

clairelin135 commented Mar 6, 2024

salazarm left a comment

salazarm left a comment

hellendag left a comment

hellendag Mar 7, 2024

hellendag Mar 7, 2024

hellendag Mar 7, 2024

clairelin135 Mar 7, 2024

hellendag Mar 7, 2024

clairelin135 Mar 7, 2024

clairelin135 commented Mar 7, 2024

New asset catalog page #20210

New asset catalog page #20210

Conversation

clairelin135 commented Mar 2, 2024 • edited Loading

github-actions bot commented Mar 2, 2024 • edited Loading

github-actions bot commented Mar 2, 2024 • edited Loading

clairelin135 commented Mar 4, 2024 • edited Loading

hellendag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 Mar 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengotow commented Mar 6, 2024 • edited Loading

clairelin135 commented Mar 6, 2024

salazarm left a comment

Choose a reason for hiding this comment

salazarm left a comment

Choose a reason for hiding this comment

hellendag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 commented Mar 7, 2024

clairelin135 commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Mar 2, 2024 •

edited

Loading

github-actions bot commented Mar 2, 2024 •

edited

Loading

clairelin135 commented Mar 4, 2024 •

edited

Loading

clairelin135 Mar 5, 2024 •

edited

Loading

clairelin135 Mar 5, 2024 •

edited

Loading

bengotow commented Mar 6, 2024 •

edited

Loading