Page MenuHomeElementl

Update Asset Detail header to reflect new Schedule header design
ClosedPublic

Authored by bengotow on Dec 21 2020, 10:53 PM.

Details

Summary

This is a small diff that adds the new "header" styling from the schedule details page to the asset details page. The goal is to make assets feel more like top level entities with their own identity and properties.

Test Plan

Run snapshot tests

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

js_modules/dagit/src/assets/AssetValueGraph.tsx
96–101

Is this roughly correct?

<Group direction="column" spacing={12}>
  <Subheading>{props.label}</Subheading>
  <Line ... />
</Group>
js_modules/dagit/src/assets/AssetView.tsx
101–109

Group here too, if not a {' '} after the link in lieu of the marginRight?

<Group direction="row" spacing={4}>
  <Link ... />
  <span>at</span>
  <Timestamp ... />
</Group>
106

Slice the run ID and make it FontFamily.monospace?

js_modules/dagit/src/sensors/SensorRoot.tsx
81–82

This change will also remove the spacing between the sections (as provided by Group) -- is that intended?

bengotow added inline comments.
js_modules/dagit/src/assets/AssetValueGraph.tsx
96–101

Yep that works, looks like it still has to be within the outer div because the Group has no intrinsic width but I guess that's fine.

js_modules/dagit/src/assets/AssetView.tsx
101–109

I'm not sure I really see the value in the Group abstraction here - adding two more layers of divs to every table row (Group => GroupChild => Timestamp, etc) to apply 4px of spacing doesn't seem super preferable. Going to swap out the marginRight for <span> at </span> to fix that spacing issue per your second suggestion!

js_modules/dagit/src/sensors/SensorRoot.tsx
81–82

Ahh good catch! Yep this still needs an inner Group.

Update asset header to use Group

This revision is now accepted and ready to land.Jan 6 2021, 4:12 PM