-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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] Bugfixes for asset tags #20723
[ui] Bugfixes for asset tags #20723
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @clairelin135 and the rest of your teammates on Graphite |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit f0d5251. |
a8f7f18
to
a2e3405
Compare
<Tag key={idx}> | ||
{tag.key}={tag.value} | ||
</Tag> | ||
<Tag key={idx}>{buildTagString({key: tag.key, value: tag.value})}</Tag> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildTagString(tag)
?
@@ -37,7 +37,7 @@ export const useAssetTagFilter = ({ | |||
/> | |||
); | |||
}, | |||
getStringValue: ({value, key}) => `${value}: ${key}`, | |||
getStringValue: ({value, key}) => `${key}: ${value}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
@@ -81,6 +81,6 @@ export function doesFilterArrayMatchValueArray<T, V>( | |||
return !filterArray.some( | |||
(filterTag) => | |||
// If no asset tags match this filter tag return true | |||
!valueArray.find((value) => !isMatch(filterTag, value)), | |||
!valueArray.find((value) => isMatch(filterTag, value)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function is used by other filters, can you confirm those still work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its only used for owners and tags, this implementation works for both
9b9c637
to
c4d72d8
Compare
a2e3405
to
8cd3fe1
Compare
Fixes a few bugs: - Makes filter value display with `key: value` instead of `value: key` - Removes an extra `!` operator to fix filtering - Adjusts asset overview page to hide `__dagster_no_value` tag values <img width="1278" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/fc15029a-88eb-4978-be01-e50c52f2422b"> <img width="1280" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/2aa51cbc-2566-4387-a9f6-2031c4ab7e52">
Fixes a few bugs:
key: value
instead ofvalue: key
!
operator to fix filtering__dagster_no_value
tag values