Page MenuHomePhabricator

New top level navigation in Dagit, rework of pipeline scope
ClosedPublic

Authored by bengotow on Dec 30 2019, 11:40 PM.

Details

Summary

This is a fairly significant rework of the Dagit navigation. A new left-hand "VSCode-style" sidebar replaces the top nav, which used pretty valuable real-estate. The "Explore" tab has been replaced by a "Pipelines" tab and the pipeline selector is within the Explore UI. The URLs have changed a bit, and the solid query is stored in the URL so that it's easy to share "views" of a pipeline and they're compatible with the browser's back button.

The Execute tab has been replaced by an execution Playground tab with a single set of saved tabs. (Previously each pipeline had it's own saved documents). The execution tab is no longer dark-themed by default (we'll probably add light/dark mode support to the entire app rather than colorizing just Execute in a dark theme.)

Minor sidenotes:

  • The solid query bar now only "commits" your typing when you press "+", "return", autocomplete a solid, or blur the field. This resolves jank when you highlight the field and immediately type "*".
  • The new UI binds Ctrl/Cmd+E to the "soft dagit reload" (we can't use Cmd+R for obvious reasons unfortunately).
  • Dagit now stores the list of pipelines in React Context because they're used in odd places and are a pain in the ass to re-org. I expect this will also come in handy when we build an "Open-Quickly" UI.

Future Todos:

This diff excludes some of the more experimental changes that we initially incorporated into the new navigation. Eventually, we want to unify the "solid query" concept and the "solid subset" used for execution so that you can view a set of solids in the Pipeline tab and click "Execute This View" to transfer that pipeline+query+mode to a new Playground session. This was mostly working but should be implemented via backend support for the solid query syntax.

We also want to explore blending the pipeline selection and the solid query into a single concept, and potentially allowing the query to select composite contents so that the entire Explore view could be represented via a query that is built/modified as you click around and drill down.

Screenshots!

Test Plan

Run updated snapshot tests

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

bengotow created this revision.Dec 30 2019, 11:40 PM
bengotow retitled this revision from New top level navigation in Dagit, rework of pipeline scope to [Pending self review] New top level navigation in Dagit, rework of pipeline scope.Dec 30 2019, 11:41 PM
bengotow removed a reviewer: Restricted Project.
bengotow edited the summary of this revision. (Show Details)Dec 30 2019, 11:43 PM
bengotow edited the summary of this revision. (Show Details)
bengotow updated this revision to Diff 8304.Dec 31 2019, 5:20 AM
  • Cleanup of diff
bengotow retitled this revision from [Pending self review] New top level navigation in Dagit, rework of pipeline scope to New top level navigation in Dagit, rework of pipeline scope.Dec 31 2019, 5:20 AM
bengotow added reviewers: alangenfeld, prha, schrockn.
bengotow updated this revision to Diff 8308.Dec 31 2019, 5:11 PM

New config picker needs to pass pipeline name to new sessions

Notes as I go:

  • I think the playground should build a url based on pipeline selected just like the pipelines tab.
  • Not for this diff, but I think we should have a new splash screen that displays instance information as well as a list of pipelines. Click on the dagster logo should go back to that. At a minimum however we have to get the instance information from something in this diff. That button is gone now. I'd take the opportunity to incorporate this into a basic splash screen that also has some explanatory text about the tool.
    • Related: In the same way I think having the "Pipelines" button open up a single pipeline. Is a little jarring. "Explore" might be better although that has its own problems. Something just feels a little off about it.
    • Also related: I think it is a bit weird to open up a pipeline automatically from the root URL.
    • If you click on the dagster logo directly it navigates to the pipelines (as I noted above I think this should navigate to a different screen) but the surrounding grey does nothing. Our click target in this menu should be consistent.
  • Keyboard short cuts for all left menu-items as well as typing in the solid subset would be great.
  • Also keyboard shortcut related, now that both the playground and the explorer have pipeline selection with identical iconography, it's confusing that there is a keyboard shortcut that works in one context but not another.
alangenfeld requested changes to this revision.Jan 2 2020, 6:12 PM

code looks reasonable, to your queue for nicks feedback

js_modules/dagit/src/PipelineExplorerRoot.tsx
24

waw

js_modules/dagit/src/execute/ExecutionTabs.tsx
109

typo

This revision now requires changes to proceed.Jan 2 2020, 6:12 PM
I think the playground should build a url based on pipeline selected just like the pipelines tab.

This is actually what I tried to do originally but it was super complex. Y'all might be able to help me think through it. The Playground tab has the concept of saved "documents", where a document is (pipeline + subset, mode, config text). It's difficult to put just the pipeline portion in the URL because it's not clear how the URL state and the document state should interact. If you switch playground tabs to a doc with another pipeline, should it change the URL? If you change the URL, should it update the current document's pipeline selection (probably not intended) or create a new document? Using the browser's back button, it's possible to "rewind" part of the document's data without rewinding the mode / preset / config text, which can very easily cause invalid combos.

I think we could get rid of the playground tabs entirely and store the entire document state in the URL (there isn't really a character limit these days afaik), which would have the upside of making the URLs easy to share, but potentially hilariously long since they'd contain the config YAML string?

Not for this diff, but I think we should have a new splash screen that displays instance information as well as a list of pipelines. Click on the dagster logo should go back to that. At a minimum however we have to get the instance information from something in this diff. That button is gone now. I'd take the opportunity to incorporate this into a basic splash screen that also has some explanatory text about the tool.

    Related: In the same way I think having the "Pipelines" button open up a single pipeline. Is a little jarring. "Explore" might be better although that has its own problems. Something just feels a little off about it.
    Also related: I think it is a bit weird to open up a pipeline automatically from the root URL.

Ahh this is a great idea - it does feel a bit off that it picks a pipeline for you.

If you click on the dagster logo directly it navigates to the pipelines (as I noted above I think this should navigate to a different screen) but the surrounding grey does nothing. Our click target in this menu should be consistent.

Will fix this!

Keyboard short cuts for all left menu-items as well as typing in the solid subset would be great.

Ahh totally, will add this!

Also keyboard shortcut related, now that both the playground and the explorer have pipeline selection with identical iconography, it's confusing that there is a keyboard shortcut that works in one context but not another.

Ahh good callβ€”I think we can address this but we'll need to change the shortcut. Right now "p" doesn't work when you're typing in the config editor for obvious reasons.

bengotow planned changes to this revision.Jan 2 2020, 10:19 PM

there isn't really a character limit these days afaik

I'm pretty sure i hit limits a few years ago with giant graphql queries in shareable graphiql URLs so I would be weary of this type of approach

bengotow updated this revision to Diff 8421.EditedJan 7 2020, 8:44 PM

Addresses diff feedback:

  • Fix spelling
  • Switch shortcuts to Opt-S and Opt-P so they work when you’re in the text editor
  • Allow you to navigate via keyboard Alt-1, Alt-2, Alt-3 through left hand tabs
  • Schedule / Scheduler => Schedules
  • Add hot little shortcut previews πŸ”₯
  • Increase logo click target size, brighten on hover similar to other tabs

This version removes some of the more experimental features but we can bring a few back in a follow-up diff.

Also planning on making the new home screen in a later diff!

  • A bit rough to only have alt show the keyboard shortcuts. 500 ms seems long too.
  • Alt-S not working for me.
  • It does feel like a bit of a regression to require alt in the explorer view.
  • While you're at it, keyboard shortcut for execution! Most useful one perhaps.

Overall this is super nice. Will let @alangenfeld or @prha review actual js

bengotow updated this revision to Diff 8425.Jan 7 2020, 9:02 PM
  • Prevent shortcuts from sticking if you tab away from the window
bengotow updated this revision to Diff 8426.Jan 7 2020, 9:13 PM
  • Prevent shortcuts from re-rendering children on on/off
bengotow updated this revision to Diff 8428.Jan 7 2020, 9:29 PM
  • Bind Alt-G to the Start button
  • Fix broken Alt-S binding in Explore tab

Hmm @schrockn I can get rid of the delay but they might get pretty annoying if they flicker in every time you use a shortcut? Definitely not supposed to be in the daily-usage path once people figure it out.

I agree re: Alt-P and Alt-S. The single key shortcuts were pretty handy. I think we could use Alt only when necessary (in the execution panel), think itd be ok to have different shortcuts in each view?

schrockn resigned from this revision.Jan 7 2020, 9:36 PM

Maybe Alt-Enter for execute?

Is there somewhere on the screen where we can put the "Alt" hint for discovering shortcuts? Pretty hard to figure out.

Fine with starting with 500 ms and seeing how it feels.

alangenfeld accepted this revision.Jan 7 2020, 10:44 PM

illallowit

js_modules/dagit/src/ShortcutHandler.tsx
102–106

hm

could you make the ref a prop to this component and do the fuckery at the callsites?

I guess i dont fully understand

but we can't be sure we get a DOM node and not a React component ref

This revision is now accepted and ready to land.Jan 7 2020, 10:44 PM

Hey hey! Rebasing this now. I initially tried for Alt-Enter, but if a dialog appears letting you know that the run could not be started / config had errors, Alt-Enter also closes the dialog so it usually just flickers. I think we could bind the event to the keyup phase of the Alt-Enter shortcut, will look and see.

js_modules/dagit/src/ShortcutHandler.tsx
102–106

Ahh that's a cool idea! Did some more reading into this -

I think this section of the docs explains this situation: https://reactjs.org/docs/refs-and-the-dom.html#exposing-dom-refs-to-parent-components. ("Accessing child DOM node to trigger focus or measuring the size or position of a child DOM node.")

It looks like we may be out of luck unless we restrict the types of nodes that can be inside this ShortcutHandler. They note in the docs

If you have absolutely no control over the child component implementation, your last option is to use findDOMNode(), but it is discouraged and deprecated in StrictMode.

I think that if we went with your approach and reworked this into a hook of some sort and forced you to attach the ref you were given by the hook to an HTML Element, that might take care of it. Will leave that for another diff for now.

bengotow updated this revision to Diff 8463.Jan 8 2020, 7:27 PM
  • Prevent shortcuts from re-rendering children on on/off
  • Merge branch 'master' of github.com:dagster-io/dagster into sfbg/layout-overhaul

Re: Is there somewhere on the screen where we can put the "Alt" hint for discovering shortcuts? Pretty hard to figure out. - let's try to put it on the new home page near the bottom, I think that screen will have an empty corner or two.