Page MenuHomePhabricator

[dagit] Core UI components
ClosedPublic

Authored by dish on Mon, Nov 9, 4:35 PM.

Details

Summary

Create a handful of very basic layout components for use in Dagit.

I don't like having to write tons of custom inline styles and styled-component CSS to get things like flexbox, padding, etc. all working the way I want them. These components will allow quicker iteration and allow folks on our team to write less CSS when working in Dagit.

Test Plan

View storybook examples. Screenshots below.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dish requested review of this revision.Mon, Nov 9, 4:41 PM

I am into it.

Also, this is the first time I've actually used Storybook. It looks pretty good!

This revision is now accepted and ready to land.Mon, Nov 9, 5:30 PM

Hey! I think this looks good. In the past I've been broadly skeptical of base components like this because I think learning how the custom DSL maps to CSS and then mentally unfolding it as you read the JSX isn't worth the cleanliness points of not writing style={display: flex, flexDirection: column} inline.

I think it'd be nice to have components targeted to specific UI (eg my attempted OptionsBar for visualizations) rather than generics that wrap common CSS, because having base components with a wide array of options doesn't prevent people from using them in /slightly/ different ways all over the app. (prettier syntax for people's random CSS :-p) I assume we can stack those on this though, and I really like what you've done with the hidden children and spacing primitives - those seem like a big win!

Hey! I think this looks good. In the past I've been broadly skeptical of base components like this because I think learning how the custom DSL maps to CSS and then mentally unfolding it as you read the JSX isn't worth the cleanliness points of not writing style={display: flex, flexDirection: column} inline.

Sorry, I should have stated my case more clearly. :)

In terms of replacing flex/margin/etc. CSS with this, the win isn't too mind-blowing, but is mostly around efficiency, typing, and documentation:

  • Type strictness on the values (vs. yolo CSS values) and vscode autocomplete on props and possible values
  • Not having to write style props, thus reducing repetitiveness and getting more of the CSS class sharing benefits of using styled-components
  • Not having to jump out of React component tree flow to write a new styled-component
  • Specific spacing values, border styles, etc. encourage consistency across the app
  • Clear Storybook examples for demonstration

Regarding Group, I've found it to be an enormously useful Swiss Army knife component for building layouts. Wrapping siblings in style={...} wrappers or applying per-child CSS often feels like a lot more work than it probably should be, and Group is a nice way to reduce that burden.

I think it'd be nice to have components targeted to specific UI (eg my attempted OptionsBar for visualizations) rather than generics that wrap common CSS, because having base components with a wide array of options doesn't prevent people from using them in /slightly/ different ways all over the app.

Yeah, I don't think we need to prevent people from doing slightly different things (4px here, 8px there) but I do think it's useful to have some guardrails, especially as an aid for people who aren't used to building UI. If we end up with subtle unwanted differences in prop values, I believe that will be easier to corral than trying to track down uncontrolled CSS styles. For more use-case-specific components, they'd ideally be quickly buildable using these basic pieces.

Some stuff should always just be done in CSS, e.g. positioning, z-indexes, etc. -- and I don't think we should ever try to bake those in.

This revision was automatically updated to reflect the committed changes.