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] Show re-bound keyboard nav hints instead of their default values #20235

Conversation

philrenaud
Copy link
Contributor

In the Nomad UI, you can re-bind your keyboard shortcuts (from g j to job to go to the jobs route, for example).

But prior to this PR, whether or not you'd re-bound a key command, the hint that would show up when holding shift would always be the default. This is because the hints are generated via {{keyboard-shortcut}} modifier, and are separate from the list of the keyboard service's defaultCommands list.

This PR does a lookup on get hints(), which fires every time the user holds shift.
image
image

Copy link

github-actions bot commented Mar 27, 2024

Ember Test Audit comparison

main 19a1468 change
passes 1550 1550 0
failures 0 0 0
flaky 0 0 0
duration 11m 05s 767ms 10m 48s 093ms -17s 674ms

@@ -110,6 +111,7 @@ export default class KeyboardService extends Service {
{
label: 'Go to Variables',
action: () => this.router.transitionTo('variables'),
rebindable: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side-effect: the "go to variables" key command wasn't re-bindable! I must've forgotten during Vars work.

@@ -209,6 +212,12 @@ export default class KeyboardService extends Service {
});
}

getCommandByPattern(pattern) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where is this one used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this is an orphaned method from my first attempt at solving this. Thank you for spotting it!

Copy link
Member

@Juanadelacuesta Juanadelacuesta left a comment

Choose a reason for hiding this comment

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

Just a question, but it looks fine

@philrenaud philrenaud added the backport/1.7.x backport to 1.7.x release line label Apr 8, 2024
@philrenaud philrenaud merged commit 9a20e98 into main Apr 8, 2024
17 checks passed
@philrenaud philrenaud deleted the 20232-ui-bug-keyboard-nav-re-binding-doesnt-show-up-in-hints-with-shift branch April 8, 2024 14:11
philrenaud added a commit that referenced this pull request Apr 18, 2024
…#20235)

* Rebinds show up as soon as you start rebinding

* Hint bind and rebind tests

* Orphaned getCommandByPattern method removed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.7.x backport to 1.7.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ui] Bug: keyboard nav re-binding doesn't show up in hints with shift
2 participants