Page MenuHomePhabricator

[dagit] Warn on tab removal in Playground
ClosedPublic

Authored by dish on Fri, Oct 9, 8:48 PM.

Details

Summary

Resolves #3066

Stacked on https://dagster.phacility.com/D4747.

Add a warning dialog when a tab is about to be removed on the Playground. This prevents accidental deletion of tabs and loss of configuration work.

Test Plan

View Playground, remove tab. Verify proper dialog rendering and behavior.

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.Fri, Oct 9, 8:53 PM

This prevents accidental deletion of tabs and loss of configuration work.

This makes sense. I personally get into a state where I have way too many tabs and need to quickly close a bunch of them. In the past, I believe dwall even suggested making the tabs an even width so he could keep his mouse in one place to quickly delete tabs. Maybe we could consider adding a modifier key that bypasses the dialogue in the future.

js_modules/dagit/src/execute/ExecutionTabs.tsx
115–121

We have a hook called useConfirmation that is used elsewhere for the same functionality here. This looks good but just wanted to make sure you knew about it.

js_modules/dagit/src/execute/ExecutionTabs.tsx
115–121

Ah, thanks, I did not know about it. I'll take a look.

useConfirmation, don't show X if only one tab

This looks great! Did not realize we had a useConfirmation hook, but that makes this super clean. (Though I assume it's stateless and doesn't only remind you on the removal of the first tab?)

This revision is now accepted and ready to land.Tue, Oct 13, 3:09 PM

This looks great! Did not realize we had a useConfirmation hook, but that makes this super clean. (Though I assume it's stateless and doesn't only remind you on the removal of the first tab?)

Yeah, it reminds on any removal -- I made a change so that when there's only one tab, there's no X.

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

Can only remove if more than one. :)

This revision was automatically updated to reflect the committed changes.