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] Display lowercase compute and storage kinds and correctly map icons #23784

Closed
wants to merge 1 commit into from

Conversation

bengotow
Copy link
Collaborator

Summary & Motivation

Fixes https://linear.app/dagster-labs/issue/FE-503/compute-kind-buttons-not-showing-proper-icon-on-catalog-when-they-are

The asset catalog was not grouping compute_kind: SQL and compute_kind: sql together. The uppercase version also did not not have a corresponding image.

I don't think compute_kind or storage_kind are ever meant to have uppercase letters, so the easiest solution to fix both the grouping and the icons is to lowercase the strings before grouping.

How I Tested These Changes

I reproduced this by specifying SQL vs sql compute_kind on a few assets. I also verified that the asset catalog search is also case insensitive.

@bengotow bengotow requested review from hellendag and salazarm August 21, 2024 03:23
Copy link

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-bvnk4qqjj-elementl.vercel.app
https://bengotow-2024-08-FE-503.core-storybook.dagster-docs.io

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

@bengotow
Copy link
Collaborator Author

Hmm based on the conflicts with this one, it seems like compute kind tags have been folded into a new / broader assetDefinition.tags - we may want to lowercase those but it seems like compute kind isn't a category so it may not need special treatment. Closing this for now!

@bengotow bengotow closed this Sep 28, 2024
@bengotow bengotow deleted the bengotow-2024-08/FE-503 branch November 21, 2024 20:28
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.

2 participants