Page MenuHomeElementl

asset tags visible in catalog table view
ClosedPublic

Authored by prha on Mar 10 2021, 12:50 AM.

Details

Summary

Test Plan

filtered asset catalog by tags

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

prha requested review of this revision.Mar 10 2021, 12:59 AM

Tags look fine to me.

A couple tangential notes:

  • I noticed is that the Page component probably needs a height: 100% to prevent small asset lists from making the container too small for the dropdown. I think it's AssetCatalogRoot.
  • The Heading breadcrumbing in AssetEntryRoot is redundant, since we have breadcrumbing in the text beneath the header.

Will let others take a look too, since I think they have more context on the product.

js_modules/dagit/src/assets/AssetsCatalogTable.tsx
282–285
const sorted = React.useMemo(() => [...assets].sort((a, b) =>
  a.toLocaleLowerCase().localeCompare(b.toLocaleLowerCase()),
  [assets],
);
js_modules/dagit/src/assets/AssetsFilter.tsx
202

This border is a bit too light. Can you use Colors.LIGHT_GRAY1?

203

Necessary?

This looks great! I think asset tags are better than expecting everyone to collect their assets into folders with the path style names. I'm a little wary of pipeline being encoded on assets via a tag because it'd be nice to have a more formal structure with the full [pipeline+mode] links we render elsewhere, but that's out of scope of this :-)

I wonder if this AssetFilter should use the new tokenizing field replacement @dish put together? Maybe that's just useSuggestionsForString and the rest is left as an exercise to the reader but it seems there's some stuff we could collect into a bigger hook or a component in a follow-up diff later.

This revision is now accepted and ready to land.Mar 15 2021, 3:24 AM
This revision was automatically updated to reflect the committed changes.