Page MenuHomePhabricator

#2128 add keyboard shortcut for preset button in the playground
ClosedPublic

Authored by yuhan on Mar 9 2020, 7:37 PM.

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

yuhan created this revision.Mar 9 2020, 7:37 PM
yuhan edited the summary of this revision. (Show Details)Mar 9 2020, 7:46 PM
yuhan edited the test plan for this revision. (Show Details)
yuhan added a reviewer: prha.Mar 9 2020, 7:50 PM
yuhan added inline comments.
js_modules/dagit/src/execute/ConfigEditorConfigPicker.tsx
238

@prha let me know if this needs to be typed better

prha requested changes to this revision.Mar 9 2020, 8:51 PM
prha added inline comments.
js_modules/dagit/src/execute/ConfigEditorConfigPicker.tsx
238

Ideally this would be typed as Button instead of any, but I can see we're trying to get around the protected buttonRef property.

I think it's hacky, but everything seems hacky. Maybe we can type it as Button, but keep the any cast in line 245 and add a comment about it?

242–243

why ⌥X? (just don't see the thread that ties X to presets or partitions). Or

what about something like ⌥R (pReset) or ⌥C (Config)?

This revision now requires changes to proceed.Mar 9 2020, 8:51 PM
yuhan updated this revision to Diff 10385.Mar 9 2020, 11:20 PM
yuhan marked 2 inline comments as done.
yuhan edited the summary of this revision. (Show Details)

update

yuhan edited the summary of this revision. (Show Details)Mar 9 2020, 11:21 PM
yuhan edited the test plan for this revision. (Show Details)
yuhan added inline comments.
js_modules/dagit/src/execute/ConfigEditorConfigPicker.tsx
242–243

⌥R is re-launch button, ⌥C is chrome's Inspect Elements.
I switched to ⌥E for (prEset)

yuhan edited the summary of this revision. (Show Details)Mar 10 2020, 4:16 AM
prha added inline comments.Mar 10 2020, 4:04 PM
js_modules/dagit/src/execute/ConfigEditorConfigPicker.tsx
245

After thinking about this more, I think reaching into the Button component protected refs is a bad idea.

The ideal solution would be to rewrite the ConfigGeneratorSelect to be a controlled component (https://reactjs.org/docs/forms.html#controlled-components) and then have the onShortcut handler just change state directly. You might want to take a look at this section of the Blueprint docs (https://blueprintjs.com/docs/#select/select-component.controlled-usage)

Short of that, it might be better to take the DOM-based approach in https://dagster.phacility.com/source/dagster/browse/master/js_modules/dagit/src/PipelineJumpComponents.tsx.

But maybe @bengotow can comment...

prha added a comment.Mar 10 2020, 4:05 PM

Okay, I think this looks okay.... at least, it's consistent with PipelineJumpComponents.tsx... would still like @bengotow to take a look...

js_modules/dagit/src/execute/ConfigEditorConfigPicker.tsx
327

remove

yuhan marked an inline comment as done.Mar 10 2020, 6:26 PM
bengotow accepted this revision.Mar 10 2020, 7:19 PM

Hey! Just wanted to chime in — I think that the final approach here (using the DOM APIs to "click" the select when the shortcut is activated) is probably the best. I ran into this with the PipelineJumpBar as well. Ideally, the select Component would expose public methods (class components can have instance methods that you can call to imperatively, eg myCustomComponentRef.zoomToCenter()) OR allow for controlled usage. Blueprint's Select component is more or less a black box, and activating it via the DOM is the least-likely-to-break hack, since it should fundamentally always respond to a click event.

I actually wonder if it might make sense to bake this behavior into the <ShortcutHandler /> itself. Maybe if no onShortcut method is provided, it could get it's own ReactDOM node, find the closest button child and click it as a default behavior? Just a thought - might not be worth it until we uncover one or two more use cases!

yuhan added a comment.Mar 10 2020, 7:40 PM

Hey! Just wanted to chime in — I think that the final approach here (using the DOM APIs to "click" the select when the shortcut is activated) is probably the best. I ran into this with the PipelineJumpBar as well. Ideally, the select Component would expose public methods (class components can have instance methods that you can call to imperatively, eg myCustomComponentRef.zoomToCenter()) OR allow for controlled usage. Blueprint's Select component is more or less a black box, and activating it via the DOM is the least-likely-to-break hack, since it should fundamentally always respond to a click event.

I actually wonder if it might make sense to bake this behavior into the <ShortcutHandler /> itself. Maybe if no onShortcut method is provided, it could get it's own ReactDOM node, find the closest button child and click it as a default behavior? Just a thought - might not be worth it until we uncover one or two more use cases!

Hey @bengotow! Thanks for the context. Ya I think it would be a lot neater to go with your proposal. But I'd still prefer to land this workaround for now because this feature doesn't look like a top priority. I will create an issue so we have this context in backlog. Lemme know if that works :)

prha accepted this revision.Mar 10 2020, 7:51 PM
This revision is now accepted and ready to land.Mar 10 2020, 7:51 PM