-
Notifications
You must be signed in to change notification settings - Fork 51
add keystrokes to run from canvas #3384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@theroinaochieng , can i have a UX review from you before I ask for a code review? |
Sure thing @taylordowns2000 taking a look in about 20 minutes. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3384 +/- ##
=======================================
Coverage 89.99% 89.99%
=======================================
Files 368 368
Lines 14458 14459 +1
=======================================
+ Hits 13011 13013 +2
+ Misses 1447 1446 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel like the right approach.
I think
- First ctrl + Enter to open the runPanel is fine living in the elixir world.
- The second ctrl + Enter to click the run button seems too off since we have to search for the button using selectors. I guess this is because the approach we have in
KeyHandlers.ts
is for registering handlers in elixir but we now have part of our code living in the JS world. Hence, let's register events for them in the JS way. in that case we wouldn't have to search for a button to click but instead call the event/function performing the action of the button directly.
* 4. If no step selected and no panel: | ||
* → Click #run-from-top button to run from trigger | ||
*/ | ||
const openRunPanelAction = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To run the workflow from the run-panel. we don't need to search for the run button using selectors which will be problematic with time. We seem to already have a function in the js world that does that. It'll be best if we can register our key events close to this function and then just call it directly instead of depending on searching for a button.
id={"workflow-edit-#{@workflow.id}"} | ||
phx-hook="OpenRunPanelViaCtrlEnter" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This keyhandler registration doesn't seem to get unregistered.
phx-hook="OpenRunPanelViaCtrlEnter" | ||
data-keybinding-scope="workflow-editor" | ||
tabindex="0" | ||
phx-mounted={JS.focus()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why we need to focus here.
]} | ||
phx-mounted={fade_in()} | ||
phx-remove={fade_out()} | ||
data-keybinding-scope="workflow-editor" | ||
tabindex="0" | ||
phx-hook="AutoFocusOnMount" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another focusing of the inspector. wondering once again.
if (isInInspector) { | ||
// Inspector mode - click the save-and-run button | ||
const runButton = document.querySelector('#save-and-run:not([disabled])'); | ||
if (runButton instanceof HTMLElement) { | ||
runButton.click(); | ||
} | ||
} else if (isInRunPanel) { | ||
// Run panel mode - click the run-from-input-selector button | ||
const runButton = document.querySelector('#run-from-input-selector:not([disabled])'); | ||
if (runButton instanceof HTMLElement) { | ||
runButton.click(); | ||
} | ||
} else if (hasStepSelected) { | ||
// Step selected but not in inspector - click the appropriate run button | ||
// Try run-from-step first (for jobs), then run-from-trigger (for triggers) | ||
const runFromStepButton = document.querySelector('#run-from-step:not([disabled])'); | ||
if (runFromStepButton instanceof HTMLElement) { | ||
runFromStepButton.click(); | ||
} else { | ||
const runFromTriggerButton = document.querySelector('#run-from-trigger:not([disabled])'); | ||
if (runFromTriggerButton instanceof HTMLElement) { | ||
runFromTriggerButton.click(); | ||
} | ||
} | ||
} else { | ||
// No step selected - trigger the same navigation as the run button | ||
const runButton = document.querySelector('#run-from-top:not([disabled])'); | ||
if (runButton instanceof HTMLElement) { | ||
runButton.click(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ids here is brittle. we might want to actually call events directly when they exist.
const hasStepSelected = url.includes('s='); | ||
const isInInspector = url.includes('m=expand'); | ||
const isInRunPanel = url.includes('m=workflow_input'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we've ever made keyHandlers depend on url params. It's mostly dependent on element that are available when we're on the screen we want the key events to be active.
eg. Undo/Redo Key handlers are registered when the workflowDiagram is mounted and unregistered when it's unmounted. Hence we wouldn't have to depend on query params.
To resolve the issue of Liveview and React both handling keyboard events (and different events at different times), we may need to build out a dedicated headless controller in React. Probably it catches the events first and decides whether to handle or delegate. Not convinced we need to do that NOW, but maybe? Let's have a design session and work out the best way forward |
Hello there @josephjclark ! Please note (pulling this in from Slack in case you're taking a peek) that the current implementation (here in this PR) works 95% the way we want but the 5% that I don't like makes me feel like something is wrong. Specifically, it all works perfectly in behavior, but in style the node (styled by This doesn't happen when you actually click the "Run" button after selecting a step. Only when you press Ctrl-Enter after selecting a step. So.... it's spooky and I don't like it. Other than that disappearing purple border on the selected node, I'm happy to merge. (But the way it disappears makes me think there's some deeper problem.) |
@taylordowns2000 Yes, there is some weird stuff in the diff around focus which may account for that. I'm 100% behind Farhan here and I wouldn't want to merge as is: that key handler makes a lot of assumptions about the shape and structure of the app, and I don't want to have to maintain it every time we add new pages, or rebuild existing ones. I don't think there's any urgency to merge bad code. I'll look at this with the team this week and we'll work out a more sustainable solution. I don't want it to be super expensive - it just needs a bit more thought |
Note: When the AI chat panel is open and the textarea is active, CTRL + ENTER is for submitting the content in the textarea. Let's not forge to test that. |
Description
This PR closes #3320 .
Merge docs when this is merged: OpenFn/docs#683
Validation steps
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner
,:admin
,:editor
,:viewer
)