Add a keyboard shortcut central registry for page views.

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

Information

Review Board
master

Reviewers

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 ID Author
Add a keyboard shortcut central registry for page views.
3dec5902e36f9da5eec25993418c4e88c667f80a Jay
Remove one extra space.
3d82ba3525b1f48711372952ec94389951542d70 Jay
Add missing semicolon.
5acf1f77a465eb609535fade44747d64bbd5e00e Jay
Remove console.log and change some wording.
e63693dc2281dd08feaf4957f3ab57717c50a342 Jay
Use extend instead of overwriting.
699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay
Made keyBindings comments more clear.
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 …

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. Show all issues

    Could change the wording from The dictionary to A dictionary

  3. Show all issues

    use shortcut instead of shortcuts

  4. Show all issues

    Try This key selects the previous comment. instead

  5. Show all issues

    Try This key selects the next file. instead

  6. Show all issues

    Try This key selects the previous diff. instead

  7. Show all issues

    Try This key selects the next diff. instead

  8. Show all issues

    Try This key selects the previous comment. instead

  9. Show all issues

    Try This key selects the next comment. instead

  10. Show all issues

    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. Show all issues

    Try This key creates a new comment. instead

  12. Show all issues

    Could remove this console.log statement

  13. 
      
LittleGrey
hxqlin
  1. 
      
  2. Show all issues

    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. Show all issues

    i think we usually use extend as opposed to expand

  4. Show all issues

    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. Show all issues

    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. Show all issues

    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. Show all issues

    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 ID Author
Add a keyboard shortcut central registry for page views.
3dec5902e36f9da5eec25993418c4e88c667f80a Jay
Remove one extra space.
3d82ba3525b1f48711372952ec94389951542d70 Jay
Add missing semicolon.
5acf1f77a465eb609535fade44747d64bbd5e00e Jay
Remove console.log and change some wording.
e63693dc2281dd08feaf4957f3ab57717c50a342 Jay
Use extend instead of overwriting.
699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay
Add a keyboard shortcut central registry for page views.
3dec5902e36f9da5eec25993418c4e88c667f80a Jay
Remove one extra space.
3d82ba3525b1f48711372952ec94389951542d70 Jay
Add missing semicolon.
5acf1f77a465eb609535fade44747d64bbd5e00e Jay
Remove console.log and change some wording.
e63693dc2281dd08feaf4957f3ab57717c50a342 Jay
Use extend instead of overwriting.
699106b138a1736c497fdb1ed23c5cbdd20d5605 Jay
Made keyBindings comments more clear.
30dcd5a21dd08fdcad24a2d9a7e83bcb52292214 Jay

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
chipx86
  1. 
      
  2. Show all issues

    "&" -> "and"

  3. reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  4. Show all issues

    "consisting"

  5. Show all issues

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

  6. 
      
chipx86
  1. 
      
  2. Show all issues

    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.