Page MenuHomeElementl

[dagit] Simplify Table styles
ClosedPublic

Authored by dish on Jan 22 2021, 8:22 PM.

Details

Summary

Simplify tables in Dagit:

  • Make all tables width: 100% to remove that from callsites
  • Remove striped usage in favor of tr-level gray keyline boxshadow
  • Set first-child and last-child table cells to be flush against the horizontal sides
  • Add a $compact transient prop to render small tables with minimal padding
  • Add a storybook example

The striped version was a step in the right direction to get us to a true table without having lots of internal borders -- as is the default in Blueprint Table -- but the stripes are still a bit cluttered.

By extending the first-child and last-child table cells to the edges, we get better alignment with headers and other page content. To that end, I added a bit more padding to Page to restore some breathing room whitespace around table edges.

Screenshots below.

Test Plan

View Storybook example, verify rendering as described above.

Load Dagit, navigate to all affected tables, verify rendering as described above.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dish requested review of this revision.Jan 22 2021, 8:30 PM

Whoaaa, not striped is by far the biggest change in this. Is the rationale the visual clutter?

We might need to consider re-generating all of our screenshots with this change...

Whoaaa, not striped is by far the biggest change in this. Is the rationale the visual clutter? We might need to consider re-generating all of our screenshots with this change...

It's a few things.

  • The stripes add visual clutter, yeah -- and if we ever want a UI where hovering on a row shows some distinct background color, the stripes will get in the way.
  • Looking around at other table views and component libraries, the approach introduced here (per-row lines, no stripes) appears pretty standard. Stripes are less common.
  • The stripes are helpful for row delineation vs. the other option provided by default in the Blueprint table component, which is per-cell borders. Per-cell borders aren't really a thing in 2021, so in switching from the "card" rows to the table component, stripes were kind of my only option without also introducing custom CSS right away, which I didn't really want to spend time on at that point.
  • By using a transparent background, it makes it easier to eliminate the horizontal spacing on the first and last cells, which significantly improves horizontal alignment with headers and other page elements. (This was probably my main motivation for the change.) If we ever have hover states on rows this could become a little awkward again but IMO it's still an improvement.

It could require some screenshot regeneration, but I'm not sure we would need to do that right away, since the UI is otherwise pretty much unchanged. No strong opinion there though.

I think this is a positive change - removing the alternating background seems fine and makes the rows appear less clickable (which I think is a *good* thing because our rows are largely not clickable.)

Screenshot regeneration does seem like a legitimate pain - we could potentially hold this until there are other visual changes (eg: brand new sidebar) and do all the new screenshots at once, since I don't think this change is needed urgently? Up to you guys though! I'm not sure what goes into the generation of the screenshots for the docs.

js_modules/dagit/src/pipelines/PipelineOverviewRoot.tsx
145

Oh interesting I hadn't seen one of these before, this is a nice library feature though I do think the $ looks super funky. (https://medium.com/@probablyup/introducing-transient-props-f35fd5203e0c for anyone else who's curious!)

This revision is now accepted and ready to land.Jan 25 2021, 7:25 PM
This revision was automatically updated to reflect the committed changes.

Went ahead and landed it since I don't want to get stale and deal with conflicts later on. I think we can either go ahead and update screenshots now, or not worry about it until 0.11.0 if/when we have more noticeable UI changes. IMO this is subtle enough that it's not super critical to update them right now.