• 
      

    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.