Add a keyboard shortcut central registry for page views.
Review Request #10913 — Created Feb. 24, 2020 and updated
Created an object which can be a central registry for keyboard
shortcuts on the current page. This new objectkeyBindings
is
owned byRB.Pageview
. Subclasses ofRB.PageView
(e.g.,diffViewerPageView
anddataGridPageView
can extend this new object to have their own
keyboard shortcuts.Each key set can map to two things:
callback
mapping to a callback
function andhelp
(temporary mapping to a str text, can be a foundation
of future features).
The old keyboard shortcuts on
diffViewerPageView
are still working.Tested on Chrome and Firefox.
Tested in
dataGridPageView
by adding this dummy code segment:... keyBindings: _.extend({ 'a': { callback: 'testAlert', } }, RB.PageView.prototype.keyBindings), testAlert() { alert("This is a test alert."); }, ...
Summary | ID | Author |
---|---|---|
3dec5902e36f9da5eec25993418c4e88c667f80a | Jay | |
3d82ba3525b1f48711372952ec94389951542d70 | Jay | |
5acf1f77a465eb609535fade44747d64bbd5e00e | Jay | |
e63693dc2281dd08feaf4957f3ab57717c50a342 | Jay | |
699106b138a1736c497fdb1ed23c5cbdd20d5605 | Jay | |
30dcd5a21dd08fdcad24a2d9a7e83bcb52292214 | Jay |
Description | From | Last Updated |
---|---|---|
just to clarify how the keyboard shortcuts work, would pressing any key in for example 'aAKP<m' trigger the shortcut? also … |
hxqlin | |
Col: 61 Missing semicolon. |
reviewbot | |
Could change the wording from The dictionary to A dictionary |
bui1 | |
use shortcut instead of shortcuts |
bui1 | |
Try This key selects the previous comment. instead |
bui1 | |
Try This key selects the next file. instead |
bui1 | |
Try This key selects the previous diff. instead |
bui1 | |
Try This key selects the next diff. instead |
bui1 | |
Try This key selects the previous comment. instead |
bui1 | |
Try This key selects the next comment. instead |
bui1 | |
I'm not totally sure what this commands does on my first read through. I think changing the wording for this … |
bui1 | |
Try This key creates a new comment. instead |
bui1 | |
Could remove this console.log statement |
bui1 | |
i think we usually use extend as opposed to expand |
hxqlin | |
not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since … |
hxqlin | |
i would prefer doc instead of help since it feels more specific but this is up to interpretation |
hxqlin | |
Nit: "help texts" -> "help text" We should also indicate what fields are expected in the dictionary. What is the … |
mike_conley | |
I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this … |
mike_conley | |
"&" -> "and" |
chipx86 | |
It would be great if this could document each key that's expected by an entry, using a format similar to … |
chipx86 | |
"consisting" |
chipx86 | |
For each of these, can you wrap the string with gettext()? That'll prepare it for localization. |
chipx86 | |
We should also leave off the "This key" part. Just like: "Select the previous file." "Select the next diff." "Re-center … |
chipx86 |
- Commits:
-
Summary ID Author 83c91ec40f1ca2246d12a403a71e6fcd5ab1cd10 Jay af28b58d503b264619fb73301209d685753fa0c7 Jay 83c91ec40f1ca2246d12a403a71e6fcd5ab1cd10 Jay af28b58d503b264619fb73301209d685753fa0c7 Jay 2d9bd989099ee5a997b8deb3a483f4edfc9e09f0 Jay
Checks run (2 succeeded)
-
Good work! I'm excited to see other pages extend their own custom keyboard shortcuts from this in the future. Just some small things that could help with wording
-
-
-
-
-
-
-
-
-
I'm not totally sure what this commands does on my first read through. I think changing the wording for this will help clear up confusion with users
-
-
- Commits:
-
Summary ID Author 83c91ec40f1ca2246d12a403a71e6fcd5ab1cd10 Jay af28b58d503b264619fb73301209d685753fa0c7 Jay 2d9bd989099ee5a997b8deb3a483f4edfc9e09f0 Jay 83c91ec40f1ca2246d12a403a71e6fcd5ab1cd10 Jay af28b58d503b264619fb73301209d685753fa0c7 Jay 2d9bd989099ee5a997b8deb3a483f4edfc9e09f0 Jay 674b9105e7e2dcc22d9b4da89040bf85ea0bc6d8 Jay
Checks run (2 succeeded)
-
-
just to clarify how the keyboard shortcuts work, would pressing any key in for example
'aAKP<m'
trigger the shortcut?also i think it would be helpful to link to any mockups or additional work related to this review request because it's not super clear from this work alone what you're trying to accomplish.
-
-
not sure but i think defining
keyBindings
like this will overwrite the parent object; it's not a problem atm since the parent object is empty, but if we end up having global key bindings they won't work on these pages because they'll be overwritten. i think you want something like this:keyBindings: _.extend({ ... }, RB.PageView.prototype.keyBindings),
-
- Testing Done:
-
The old keyboard shortcuts on
diffViewerPageView
are still working.Tested on Chrome and Firefox.
Tested in
dataGridPageView
by adding this dummy code segment:...
~ keyBindings: {
~ keyBindings: _.extend({
'a': {
callback: 'testAlert',
}
~ },
~ }, RB.PageView.prototype.keyBindings),
testAlert() {
alert("This is a test alert.");
},
- ...
- Commits:
-
Summary ID Author 83c91ec40f1ca2246d12a403a71e6fcd5ab1cd10 Jay af28b58d503b264619fb73301209d685753fa0c7 Jay 2d9bd989099ee5a997b8deb3a483f4edfc9e09f0 Jay 674b9105e7e2dcc22d9b4da89040bf85ea0bc6d8 Jay 3dec5902e36f9da5eec25993418c4e88c667f80a Jay 3d82ba3525b1f48711372952ec94389951542d70 Jay 5acf1f77a465eb609535fade44747d64bbd5e00e Jay e63693dc2281dd08feaf4957f3ab57717c50a342 Jay 699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay
Checks run (2 succeeded)
-
Hi Xiaohui,
This looks like a great approach to me! Just a few minor suggestions.
-
Nit: "help texts" -> "help text"
We should also indicate what fields are expected in the dictionary. What is the key supposed to be? What can the values be?
-
I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this instead of replacing it".
- Commits:
-
Summary ID Author 3dec5902e36f9da5eec25993418c4e88c667f80a Jay 3d82ba3525b1f48711372952ec94389951542d70 Jay 5acf1f77a465eb609535fade44747d64bbd5e00e Jay e63693dc2281dd08feaf4957f3ab57717c50a342 Jay 699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay 3dec5902e36f9da5eec25993418c4e88c667f80a Jay 3d82ba3525b1f48711372952ec94389951542d70 Jay 5acf1f77a465eb609535fade44747d64bbd5e00e Jay e63693dc2281dd08feaf4957f3ab57717c50a342 Jay 699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay 30dcd5a21dd08fdcad24a2d9a7e83bcb52292214 Jay