Skip to content
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

[ui] Perform common job tasks with keyboard shortcuts #16378

Merged
merged 5 commits into from
Mar 20, 2023

Conversation

philrenaud
Copy link
Contributor

Traversal has been a strong-suit of keyboard navigation for long enough that I feel confident in its behaviour in the Nomad UI.

Now, this PR adds some basic action-running for jobs.

From the jobs index:

  • run a new job with r u n

From a job page:

  • exec with e x e c
  • stop with s t o p
  • purge with p u r g e
  • start with s t a r t

image

@philrenaud philrenaud requested review from lgfa29 and mikenomitch March 7, 2023 21:25
@philrenaud philrenaud self-assigned this Mar 7, 2023
@@ -411,6 +411,7 @@ export default class KeyboardService extends Service {
command.label === 'Show Keyboard Shortcuts' ||
command.label === 'Hide Keyboard Shortcuts'
) {
event.preventDefault();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this so that the letter n (from having typed run) doesn't show up in the job template editor box (keyboard service eagerly performs a task on keydown, but typing waits for keyup)

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Ember Asset Size action

As of ba6f522

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +1.55 kB +316 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Ember Test Audit comparison

main ba6f522 change
passes 1485 1485 0
failures 0 0 0
flaky 0 0 0
duration 10m 55s 187ms 10m 57s 014ms +01s 827ms

Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the label is the main thing that needs to be fixed (though I'm not sure what's the impact of having them wrong).

As for the confirmation I would gather a second opinion. My spidey-sense makes me worry a little about them in production.

(Also don't forget a changelog entry and the backport label 🙂)

ui/app/templates/components/job-page/parts/title.hbs Outdated Show resolved Hide resolved
Comment on lines 42 to 46
{{keyboard-shortcut
label="Exec"
pattern=(array "p" "u" "r" "g" "e")
action=(perform this.purgeJob)
}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a pretty long shortcut, but I'm kind of wary about the potential for unintended destructive operations here. Would it be possible to still have the confirmation pop-up? Keyboard shortcuts make things quick but sometimes it's good to slow down and double-check you are in the right namespace or something like that 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been giving this some thought. A couple things:

  • Our two-step buttons are probably not a convention that we're going to continue with in the near future; Helios indicates that we should either use modals to warn, or toast/modals to make undoing easy. Once we start using modals, adding them to a queue of keyboard shortcut actions will be possible, but still not an ideal flow. As it stands now, the visual effect probably wouldn't be enough to make the user notice and they might have assumed the purge command went through without noticing otherwise.
  • The act of typing the full word is a preeeeeetty deliberate action - something much harder to do than accidentally mouse-clicking or tab+entering on the button while enroute to do something else on the page.

As such, I think I'd like to move forward with this change. I am worried about the "double-checking you're in the right namespace" problem as well but the benefit here seems to outweigh the potential cost.

ui/app/templates/components/job-page/parts/title.hbs Outdated Show resolved Hide resolved
Copy link
Contributor

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's one typo that should be fixed before you merge this.

ui/app/components/job-page/parts/title.js Outdated Show resolved Hide resolved
ui/app/components/job-page/parts/title.js Outdated Show resolved Hide resolved
ui/app/components/job-page/parts/title.js Outdated Show resolved Hide resolved
ui/app/controllers/jobs/index.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.5.x backport to 1.5.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants