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

Dynamic numeric keynav for multiple tables #13482

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file.
5 changes: 2 additions & 3 deletions ui/app/components/keyboard-shortcuts-modal.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import Component from '@glimmer/component';
import { inject as service } from '@ember/service';
import { htmlSafe } from '@ember/template';
import { computed } from '@ember/object';
import Tether from 'tether';

Expand Down Expand Up @@ -28,7 +27,7 @@ export default class KeyboardShortcutsModalComponent extends Component {
* hints: filter keyCommands to those that have an element property,
* and then compute a position on screen to place the hint.
*/
@computed('keyboard.keyCommands.length', 'keyboard.displayHints')
@computed('keyboard.{keyCommands.length,displayHints}')
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Why not just declare length and displayHints as tracked properties here? Are we relying on computed properties for a cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you expand on this a bit? What is the benefit to having these as tracked properties of the modal vs global state in the service?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a Glimmer component and the long-term plan is moving to a pull-based reactivity model (tracked properties) and fully away (deprecating) from a push-based reactivity model (computeds, observables, two-way bindings).

get hints() {
if (this.keyboard.displayHints) {
return this.keyboard.keyCommands.filter((c) => c.element);
Expand All @@ -47,7 +46,7 @@ export default class KeyboardShortcutsModalComponent extends Component {
});
hint.binder = binder;
}
untetherFromElement(self, _, { element, hint }) {
untetherFromElement(self, _, { hint }) {
hint.binder.destroy();
}
}
2 changes: 2 additions & 0 deletions ui/app/controllers/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import codesForError from '../utils/codes-for-error';
import NoLeaderError from '../utils/no-leader-error';
import OTTExchangeError from '../utils/ott-exchange-error';
import classic from 'ember-classic-decorator';
// eslint-disable-next-line no-unused-vars
import KeyboardService from '../services/keyboard';
@classic
export default class ApplicationController extends Controller {
Expand All @@ -21,6 +22,7 @@ export default class ApplicationController extends Controller {
*/
@service keyboard;

// eslint-disable-next-line ember/classic-decorator-hooks
constructor() {
super(...arguments);
this.keyboard.listenForKeypress();
Expand Down
2 changes: 0 additions & 2 deletions ui/app/helpers/keyboard-commands.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,10 @@ export default class keyboardCommands extends Helper {
@service keyboard;

constructor() {
console.log('kc const', ...arguments);
super(...arguments);
}

compute([commands]) {
console.log('computing', commands);
if (commands) {
this.commands = commands;
this.keyboard.addCommands(commands);
Expand Down
41 changes: 14 additions & 27 deletions ui/app/modifiers/keyboard-shortcut.js
Original file line number Diff line number Diff line change
@@ -1,46 +1,33 @@
import { inject as service } from '@ember/service';
import Modifier from 'ember-modifier';
import { registerDestructor } from '@ember/destroyable';
import { assert } from '@ember/debug';

export default class KeyboardShortcutModifier extends Modifier {
@service keyboard;
@service router;
philrenaud marked this conversation as resolved.
Show resolved Hide resolved

/**
* For Dynamic/iterative keyboard shortcuts, our patterns look like "Shift+0" by default
* Do a couple things to make them more human-friendly:
* 1. Make them 1-based, instead of 0-based
* 2. Prefix numbers 1-9 with "0" to make it so "Shift+10" doesn't trigger "Shift+1" then "0", etc.
* ^--- stops being a good solution with 100+ row lists/tables, but a better UX than waiting for shift key-up otherwise
*
* @param {string[]} pattern
*/
cleanPattern(pattern) {
let patternNumber = pattern.length === 1 && pattern[0].match(/\d+/g);
if (!patternNumber) {
return pattern;
} else {
patternNumber = +patternNumber[0]; // regex'd string[0] to num
patternNumber = patternNumber + 1; // first item should be Shift+1, not Shift+0
assert(
'Dynamic keyboard shortcuts only work up to 99 digits',
patternNumber < 100
);
pattern = [`Shift+${('0' + patternNumber).slice(-2)}`]; // Shift+01, not Shift+1
modify(
element,
_positional,
{
label,
pattern = '',
action = () => {},
menuLevel = false,
enumerated = false,
philrenaud marked this conversation as resolved.
Show resolved Hide resolved
}
return pattern;
}

modify(element, [eventName], { label, pattern, action, menuLevel = false }) {
) {
let commands = [
{
label,
action,
pattern: this.cleanPattern(pattern),
pattern,
element,
menuLevel,
enumerated,
},
];

this.keyboard.addCommands(commands);
registerDestructor(this, () => {
this.keyboard.removeCommands(commands);
Expand Down
53 changes: 40 additions & 13 deletions ui/app/services/keyboard.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
// @ts-check
import Service from '@ember/service';
import { inject as service } from '@ember/service';
import { timeout, restartableTask } from 'ember-concurrency';
import { tracked } from '@glimmer/tracking';
import { compare } from '@ember/utils';
import { A } from '@ember/array';
// eslint-disable-next-line no-unused-vars
import EmberRouter from '@ember/routing/router';
import { schedule } from '@ember/runloop';
import { action } from '@ember/object';
import { guidFor } from '@ember/object/internals';
import { assert } from '@ember/debug';
// eslint-disable-next-line no-unused-vars
import MutableArray from '@ember/array/mutable';

const DEBOUNCE_MS = 750;

Expand Down Expand Up @@ -37,7 +42,10 @@ export default class KeyboardService extends Service {
@tracked buffer = A([]);
@tracked displayHints = false;

keyCommands = [
/**
* @type {MutableArray<Object>}
*/
keyCommands = A([
{
label: 'Go to Jobs',
pattern: ['g', 'j'],
Expand Down Expand Up @@ -144,26 +152,45 @@ export default class KeyboardService extends Service {
console.log('Extra Lives +30');
},
},
];
]);

addCommands(commands) {
// Filter out those commands that don't have a label (they're only being added for at-a-glance hinting/highlights)
this.keyCommands.pushObjects(commands);
/**
* For Dynamic/iterative keyboard shortcuts, we want to do a couple things to make them more human-friendly:
* 1. Make them 1-based, instead of 0-based
* 2. Prefix numbers 1-9 with "0" to make it so "Shift+10" doesn't trigger "Shift+1" then "0", etc.
* ^--- stops being a good solution with 100+ row lists/tables, but a better UX than waiting for shift key-up otherwise
*
* @param {number} iter
* @returns {string[]}
*/
cleanPattern(iter) {
iter = iter + 1; // first item should be Shift+1, not Shift+0
assert('Dynamic keyboard shortcuts only work up to 99 digits', iter < 100);
return [`Shift+${('0' + iter).slice(-2)}`]; // Shift+01, not Shift+1
}

removeCommands(commands) {
this.keyCommands.removeObjects(commands);
recomputeEnumeratedCommands() {
this.keyCommands.filterBy('enumerated').forEach((command, iter) => {
command.pattern = this.cleanPattern(iter);
});
}

@action
generateIteratorShortcut(element, [action, iter]) {
this.keyCommands.pushObject({
label: `Hit up item ${iter}`,
pattern: [`Shift+${iter}`],
action,
addCommands(commands) {
schedule('afterRender', () => {
commands.forEach((command) => {
this.keyCommands.pushObject(command);
if (command.enumerated) {
// Recompute enumerated numbers to handle things like sort
this.recomputeEnumeratedCommands();
}
});
});
}

removeCommands(commands = A([])) {
this.keyCommands.removeObjects(commands);
}

//#region Nav Traversal

subnavLinks = [];
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/allocations/allocation/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,10 @@
</t.head>
<t.body as |row|>
<TaskRow
{{keyboard-shortcut
enumerated=true
action=(action "taskClick" row.model.allocation row.model)
}}
@data-test-task-row={{row.model.name}}
@task={{row.model}}
@onClick={{action "taskClick" row.model.allocation row.model}}
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/clients/client/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,10 @@
</t.head>
<t.body as |row|>
<AllocationRow
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
@allocation={{row.model}}
@context="node"
@onClick={{action "gotoAllocation" row.model}}
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/clients/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,13 @@
<th># Volumes</th>
<th># Allocs</th>
</t.head>
<t.body as |row iter|>
<t.body as |row|>
<ClientNodeRow
data-test-client-node-row
@node={{row.model}}
@onClick={{action "gotoNode" row.model}}
{{keyboard-shortcut
pattern=(array (concat "Shift+" iter))
enumerated=true
action=(action "gotoNode" row.model)
}}
/>
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/components/job-page/parts/children.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@
Summary
</th>
</t.head>
<t.body @key="model.id" as |row iter|>
<JobRow data-test-job-row @job={{row.model}} @context="child" @iter={{iter}} />
<t.body @key="model.id" as |row|>
<JobRow data-test-job-row @job={{row.model}} @context="child" />
</t.body>
</ListTable>
<div class="table-foot">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
@allocation={{row.model}}
@context="job"
@onClick={{action "gotoAllocation" row.model}}
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
/>
</t.body>
</ListTable>
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/components/job-page/parts/task-groups.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
@data-test-task-group={{row.model.name}}
@taskGroup={{row.model}}
@onClick={{fn this.gotoTaskGroup row.model}}
{{keyboard-shortcut
enumerated=true
action=(fn this.gotoTaskGroup row.model)
}}
/>
</t.body>
</ListTable>
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/job-row.hbs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<td data-test-job-name
{{keyboard-shortcut
pattern=(array (concat "Shift+" @iter))
enumerated=true
action=(action "gotoJob" @job)
}}
>
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/components/server-agent-row.hbs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<td data-test-server-name
{{keyboard-shortcut
pattern=(array (concat "Shift+" @iter))
enumerated=true
action=(action this.goToAgent)
}}
><LinkTo @route="servers.server" @model={{this.agent.id}} class="is-primary">{{this.agent.name}}</LinkTo></td>
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/csi/volumes/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -67,15 +67,15 @@
# Allocs
</th>
</t.head>
<t.body @key="model.name" as |row iter|>
<t.body @key="model.name" as |row|>
<tr
class="is-interactive"
data-test-volume-row
{{on "click" (action "gotoVolume" row.model)}}
>
<td data-test-volume-name
{{keyboard-shortcut
pattern=(array (concat "Shift+" iter))
enumerated=true
action=(action "gotoVolume" row.model)
}}
>
Expand Down
8 changes: 8 additions & 0 deletions ui/app/templates/csi/volumes/volume.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@
</t.head>
<t.body as |row|>
<AllocationRow
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
@data-test-write-allocation={{row.model.id}}
@allocation={{row.model}}
@context="volume"
Expand Down Expand Up @@ -90,6 +94,10 @@
</t.head>
<t.body as |row|>
<AllocationRow
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
@data-test-read-allocation={{row.model.id}}
@allocation={{row.model}}
@context="volume"
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/evaluations/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
Placement Failures
</th>
</t.head>
<t.body as |row iter|>
<t.body as |row|>
<tr
data-test-evaluation="{{row.model.shortId}}"
style="cursor: pointer;"
Expand All @@ -78,7 +78,7 @@
>
<td data-test-id
{{keyboard-shortcut
pattern=(array (concat "Shift+" iter))
enumerated=true
action=(fn this.handleEvaluationClick row.model)
}}
>
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/jobs/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,8 @@
Summary
</th>
</t.head>
<t.body @key="model.id" as |row iter|>
<JobRow @data-test-job-row={{row.model.plainId}} @job={{row.model}} @iter={{iter}} />
<t.body @key="model.id" as |row|>
<JobRow @data-test-job-row={{row.model.plainId}} @job={{row.model}} />
</t.body>
</ListTable>
<div class="table-foot">
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/jobs/job/allocations.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@
</t.head>
<t.body as |row|>
<AllocationRow
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
@data-test-allocation={{row.model.id}}
@allocation={{row.model}}
@context="job"
Expand Down
4 changes: 4 additions & 0 deletions ui/app/templates/jobs/job/task-group.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,10 @@
</t.head>
<t.body @key="model.id" as |row|>
<AllocationRow
{{keyboard-shortcut
enumerated=true
action=(action "gotoAllocation" row.model)
}}
@data-test-allocation={{row.model.id}}
@allocation={{row.model}}
@context="taskGroup"
Expand Down
4 changes: 2 additions & 2 deletions ui/app/templates/servers/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
<t.sort-by @prop="datacenter">Datacenter</t.sort-by>
<t.sort-by @prop="version">Version</t.sort-by>
</t.head>
<t.body as |row iter|>
<ServerAgentRow data-test-server-agent-row @agent={{row.model}} @iter={{iter}} />
<t.body as |row|>
<ServerAgentRow data-test-server-agent-row @agent={{row.model}} />
</t.body>
</ListTable>
<div class="table-foot">
Expand Down
Loading