Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ui] introduce pill for storage_kind tag across UI #22031

Merged
merged 4 commits into from
May 28, 2024

Conversation

benpankow
Copy link
Member

@benpankow benpankow commented May 22, 2024

Summary

Implements a very basic visual tag for storage_kind on assets. Right now, this special rendering only displays on the catalog page, asset list, and asset graph. A stacked PR handles adding the filtering.

Uses the same iconset as compute kind.

Screenshot 2024-05-22 at 2 09 55 PM Screenshot 2024-05-22 at 1 33 08 PM Screenshot 2024-05-22 at 1 33 04 PM Screenshot 2024-05-22 at 2 09 43 PM

Test Plan

Tested locally.

@benpankow
Copy link
Member Author

benpankow commented May 22, 2024

@benpankow benpankow changed the title Introduce storage kind tag, helpers to @asset, @multi_asset [ui] basic catalog view for storage_kind tag May 22, 2024
@benpankow benpankow changed the base branch from master to benpankow/storage-kind May 22, 2024 17:22
@benpankow benpankow requested review from bengotow, sryza, salazarm and hellendag and removed request for bengotow May 22, 2024 17:25
@sryza sryza requested a review from braunjj May 22, 2024 17:27
salazarm
salazarm previously approved these changes May 22, 2024
Copy link
Contributor

@salazarm salazarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@braunjj
Copy link
Contributor

braunjj commented May 22, 2024

super excited about this, but I think we should make sure the storage kind tags have parity with compute kind. IMO this means that I can:

  • See both tags on assets in the graph
  • See both tags on the asset detail page (already shown here)
  • See both tags on the assets in the list view
  • Filter the graph and list view by storage kind
  • Search by Storage Kind

LMK if you need some UI mocks for this. but I think in most cases we can just show the two tags side by side and provide a tooltip to help users discern Compute Kind vs Storage kind. I think the hover state is important because some assets might have Snowflake as both compute and storage kinds.

@braunjj
Copy link
Contributor

braunjj commented May 22, 2024

image
I think we should stack these instead of side-by-side

@benpankow
Copy link
Member Author

Updated to cover the major places where we display compute kind & added tooltips. Stacked a PR to add filtering.

@benpankow benpankow changed the title [ui] basic catalog view for storage_kind tag [ui] basic catalog view for storage_kind tag May 22, 2024
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 2853f83 to 24e7ff7 Compare May 22, 2024 22:31
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 24e7ff7 to 4241620 Compare May 22, 2024 22:32
Copy link

github-actions bot commented May 22, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-3rvpp5a58-elementl.vercel.app
https://benpankow-storage-kind-frontend-basic.core-storybook.dagster-docs.io

Built with commit 88deca2.
This pull request is being automatically deployed with vercel-action

@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 4241620 to b69358a Compare May 22, 2024 23:44
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from b69358a to 5a459bd Compare May 23, 2024 03:47
@@ -63,7 +68,18 @@ export const AssetNode = React.memo(({definition, selected}: Props) => {
<AssetNodeChecksRow definition={definition} liveData={liveData} />
)}
</AssetNodeBox>
<AssetComputeKindTag definition={definition} style={{right: -2, paddingTop: 7}} />
<Box style={{right: -2}} flex={{direction: 'row-reverse', gap: 8}}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the negative thing is weird? Does it even do anything? I would assume it doesn't since this element isnt positioned absolutely

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah good call idt it does anything

Comment on lines 73 to 74
window.requestAnimationFrame(() =>
document.dispatchEvent(new Event('show-kind-info')),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like a race condition waiting to happen but its an existing issue

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is in use anyway, will remove for storage kind.

@sryza
Copy link
Contributor

sryza commented May 23, 2024

When you say "catalog page", do you mean the asset details page or the catalog overview page that includes the search bar? I don't see a screenshot for the latter, and I think we should make sure to include a section on that page. Or is that in a different repo?

@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 5a459bd to 486a2b3 Compare May 23, 2024 21:03
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 486a2b3 to 06b0c88 Compare May 24, 2024 17:52
@benpankow benpankow force-pushed the benpankow/storage-kind branch 2 times, most recently from a0e5621 to e48701c Compare May 24, 2024 20:09
Base automatically changed from benpankow/storage-kind to master May 24, 2024 20:12
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 06b0c88 to df7fad4 Compare May 24, 2024 20:35
@benpankow
Copy link
Member Author

benpankow commented May 24, 2024

When you say "catalog page", do you mean the asset details page or the catalog overview page that includes the search bar? I don't see a screenshot for the latter, and I think we should make sure to include a section on that page. Or is that in a different repo?

That's in the internal repo, will put up a PR for it - https://github.com/dagster-io/internal/pull/9902

@benpankow benpankow changed the title [ui] basic catalog view for storage_kind tag [ui] introduce pill for storage_kind tag across UI May 24, 2024
@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from 775f524 to b1d13cb Compare May 28, 2024 21:35
@benpankow
Copy link
Member Author

benpankow commented May 28, 2024

Merge activity

  • May 28, 4:19 PM PDT: @benpankow started a stack merge that includes this pull request via Graphite.
  • May 28, 4:20 PM PDT: Graphite rebased this pull request as part of a merge.
  • May 28, 4:22 PM PDT: @benpankow merged this pull request with Graphite.

@benpankow benpankow force-pushed the benpankow/storage-kind-frontend-basic branch from b1d13cb to 88deca2 Compare May 28, 2024 23:19
@benpankow benpankow merged commit 5ad3b82 into master May 28, 2024
1 of 2 checks passed
@benpankow benpankow deleted the benpankow/storage-kind-frontend-basic branch May 28, 2024 23:22
danielgafni pushed a commit to danielgafni/dagster that referenced this pull request Jun 18, 2024
## Summary

Implements a very basic visual tag for `storage_kind` on assets. Right now, this special rendering only displays on the catalog page, asset list, and asset graph. A stacked PR handles adding the filtering.

Uses the same iconset as compute kind.

<img width="304" alt="Screenshot 2024-05-22 at 2 09 55 PM" src="https://github.com/dagster-io/dagster/assets/10215173/7988910f-642d-4f00-9753-3b0a156079a3">


<img width="391" alt="Screenshot 2024-05-22 at 1 33 08 PM" src="https://github.com/dagster-io/dagster/assets/10215173/0845ce88-e198-4dfb-a347-9cd7a949ea08">


<img width="1449" alt="Screenshot 2024-05-22 at 1 33 04 PM" src="https://github.com/dagster-io/dagster/assets/10215173/1e255c51-a5f1-4365-9b96-16121247a648">

<img width="258" alt="Screenshot 2024-05-22 at 2 09 43 PM" src="https://github.com/dagster-io/dagster/assets/10215173/8918e281-2bc0-48fb-ae03-748ef7a92fce">


## Test Plan

Tested locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants