Add a keyboard shortcut central registry for page views.

Review Request #10913 — Created Feb. 24, 2020 and updated

LittleGrey
Review Board
master
10929
reviewboard

Created an object which can be a central registry for keyboard
shortcuts on the current page. This new object keyBindings is
owned by RB.Pageview. Subclasses of RB.PageView (e.g., diffViewerPageView
and dataGridPageView 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 and help (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 Author
Add a keyboard shortcut central registry for page views.
Jay
Remove one extra  space.
Jay
Add missing semicolon.
Jay
Remove console.log and change some wording.
Jay
Use extend instead of overwriting.
Jay
Made keyBindings comments more clear.
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 ...

hxqlinhxqlin

Col: 61 Missing semicolon.

reviewbotreviewbot

Could change the wording from The dictionary to A dictionary

bui1bui1

use shortcut instead of shortcuts

bui1bui1

Try This key selects the previous comment. instead

bui1bui1

Try This key selects the next file. instead

bui1bui1

Try This key selects the previous diff. instead

bui1bui1

Try This key selects the next diff. instead

bui1bui1

Try This key selects the previous comment. instead

bui1bui1

Try This key selects the next comment. instead

bui1bui1

I'm not totally sure what this commands does on my first read through. I think changing the wording for this ...

bui1bui1

Try This key creates a new comment. instead

bui1bui1

Could remove this console.log statement

bui1bui1

i think we usually use extend as opposed to expand

hxqlinhxqlin

not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since ...

hxqlinhxqlin

i would prefer doc instead of help since it feels more specific but this is up to interpretation

hxqlinhxqlin

Nit: "help texts" -> "help text" We should also indicate what fields are expected in the dictionary. What is the ...

mike_conleymike_conley

I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this ...

mike_conleymike_conley

"&" -> "and"

chipx86chipx86

It would be great if this could document each key that's expected by an entry, using a format similar to ...

chipx86chipx86

"consisting"

chipx86chipx86

For each of these, can you wrap the string with gettext()? That'll prepare it for localization.

chipx86chipx86

We should also leave off the "This key" part. Just like: "Select the previous file." "Select the next diff." "Re-center ...

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 passed.
JSHint failed.

JSHint

LittleGrey
bui1
  1. 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

  2. Could change the wording from The dictionary to A dictionary

  3. use shortcut instead of shortcuts

  4. Try This key selects the previous comment. instead

  5. Try This key selects the next file. instead

  6. Try This key selects the previous diff. instead

  7. Try This key selects the next diff. instead

  8. Try This key selects the previous comment. instead

  9. Try This key selects the next comment. instead

  10. 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

  11. Try This key creates a new comment. instead

  12. Could remove this console.log statement

  13. 
      
LittleGrey
hxqlin
  1. 
      
  2. 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.

    1. Yes, the shortcut will be triggered by pressing any key in aAKP<m. This project is not to accomplish something, it's only for building a foundation for page views so they can have shortcuts easily. I will attach my works related to this one in the future.

  3. i think we usually use extend as opposed to expand

  4. 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),
    
  5. i would prefer doc instead of help since it feels more specific but this is up to interpretation

  6. 
      
LittleGrey
mike_conley
  1. Hi Xiaohui,

    This looks like a great approach to me! Just a few minor suggestions.

  2. 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?

  3. I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this instead of replacing it".

  4. 
      
LittleGrey
Review request changed

Commits:

Summary Author
-
Add a keyboard shortcut central registry for page views.
Jay
-
Remove one extra  space.
Jay
-
Add missing semicolon.
Jay
-
Remove console.log and change some wording.
Jay
-
Use extend instead of overwriting.
Jay
+
Add a keyboard shortcut central registry for page views.
Jay
+
Remove one extra  space.
Jay
+
Add missing semicolon.
Jay
+
Remove console.log and change some wording.
Jay
+
Use extend instead of overwriting.
Jay
+
Made keyBindings comments more clear.
Jay

Diff:

Revision 5 (+129 -49)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2. reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     

    It would be great if this could document each key that's expected by an entry, using a format similar to our "Args".

  3. For each of these, can you wrap the string with gettext()? That'll prepare it for localization.

  4. 
      
chipx86
  1. 
      
  2. We should also leave off the "This key" part. Just like:

    • "Select the previous file."
    • "Select the next diff."
    • "Re-center the selected diff chunk."
    • "Create a comment."

    etc.

  3. 
      
Loading...