Tab-based shortcuts on review request page fields.

Review Request #10929 — Created Feb. 29, 2020 and updated

Information

Review Board
master

Reviewers

This review request is for the 2nd part of "selecting review request
field" project. The goal of this project is to have tab-based keybaord
shortcuts on review request page fields. (e.g., hitting tab-d focuses
"Description" field).

This project is dependent on "Keyboard shortcut registry"

The project includes a ShortcutHelpDialog which dynamically renders
available keyboard shortcuts on each page as well as their descriptions.
A screenshot is attached below.

Press tab and h on review request page and the dialog is shown (the
shortcuts are displayed as well).

Press tab and d on review request page and Description field is
focused.

Press tab and h on dashboard page and the help dialog is shown.

All unit tests passed.

Summary ID Author
Add a keyboard shortcut central registry for page views.
20d4da8c4268f33c102486061da94fc8a66af18c Jay
Remove one extra space.
ff48beaa3d798a3d2fe07b612c7d561064db47f7 Jay
Add missing semicolon.
a324636d864dd8215f692e85425d4e2cef4d0ef4 Jay
Remove console.log and change some wording.
18847f629b301aeb2de567989234ed6690b0b73a Jay
Use extend instead of overwriting.
6e8c74bbf63591f72a84589f93add4a54a063020 Jay
Made keyBindings comments more clear.
a2a94100711811603e545a28e1d2ce5c37db92dd Jay
[WIP] Tab-based shortcuts on review request page fields.
1c9b40db935e92a8239a869cbbe14f5cf1388a10 Jay
Remove traling spaces.
538fe4586645ff2d69019858bd33e90c9edacf62 Jay
[WIP] Tab-based shortcuts on review request page.
a215d8037abd54564052af460459467f9c164ca6 Jay
Dynamically render keyboard shortcuts on each page
f0a4818f3dd6714626210881a0cf756f2873928a Jay
Better styles on shortcur help dialog.
025f1a15417988514668e3f4d1c8ffd5da988a74 Jay
Tab based shortcuts selecting fields & unit tests.
12de8777b68d977c3f5da45af4cac9eebe0487c2 Jay
Unit tests done.
9ee4a792a8e1659f08a965ef0ad488b07286acb4 Jay
Address feedback - mostly improvements.
f2f0041bfb6468fad5189b7cf464855befd67196 Jay

Description From Last Updated

We're trying to get rid of most of common.less and move to the CSS Component structure. This would be best …

chipx86chipx86

Can you wrap this in gettext() so the string can be localized?

chipx86chipx86

Looks like this would conflict with the other review request, right? That uses "docs," this uses "description."

chipx86chipx86

You can use $.ui.keyCode.TAB and 'H'.charCodeAt(0), respectively. It'd also be nice to be consistent with "KeyCode", rather than "Keycode".

chipx86chipx86

You can make use of chaining here: pageView.$el .trigger(....) .trigger(....) ....;

chipx86chipx86

It'd be nice to make these private methods (prefix with a _).

chipx86chipx86

This should use _.isEmpty().

chipx86chipx86

These must be localized. You'll want to pass in the localized strings as parameters to the template.

chipx86chipx86

Right now, we should be using _.each() instead of Object.entries().forEach(). It has wider browser compatibility. We also cannot use fat …

chipx86chipx86

You should use if (entry[1].tabBaseCallback). We want to test if it's present and has a value, and this is the …

chipx86chipx86

Same here.

chipx86chipx86

We have to use var here, for browser compatibility.

chipx86chipx86

Missing a semicolon.

chipx86chipx86

This also needs to be localized.

chipx86chipx86

This should be const (and appear as the first variable -- we do defined variables and consts before undefined and …

chipx86chipx86

I don't think we need to call .render() anymore. .show() does this for us, iirc.

chipx86chipx86

We don't re-define this, so we should use const.

chipx86chipx86

Comments should go on their own line and be proper sentences (capitalization, punctuation, "the", etc.).

chipx86chipx86

Let's use _.size() here. It will guarantee the right thing for the browser it's running on.

chipx86chipx86

Two things that can improve this: Let's pull out innerText into a variable, so we can check it without re-fetching …

chipx86chipx86

Mind adding a comment about how this state is managed?

chipx86chipx86

We should just reference $.ui.keyCode.TAB.

chipx86chipx86

Instead of _.bind(), we should use =>.

chipx86chipx86

You can chain these: this.$el .on(...) .on(...) ...; The binding below can also be included in the chain.

chipx86chipx86

This is missing an "Args" section, documenting evt and isTabBasedShortcut.

chipx86chipx86

Docstring summaries must be a single line. How about: /** * Call a shortcut handler when a key or combination …

chipx86chipx86

This can be: const keyBinding = this.keyBindings[keys]; let func = (isTabBasedShortcut ? keyBinding.tabBasedShortcut : keyBinding.callback);

chipx86chipx86

Typo: "field"

chipx86chipx86

Can you place this alphabetically in the list of test files?

chipx86chipx86
LittleGrey
LittleGrey
LittleGrey
LittleGrey
LittleGrey
LittleGrey
LittleGrey
chipx86
  1. This is looking good!

    The one thing that stood out once I went through this is that this is really three changes in one:

    1. General support for tab-based shortcuts
    2. The shortcut dialog
    3. The tab support for review request fields

    It'd really help to split them up so it's easier to test and land parts. Especially since I think the tab-based shortcuts are pretty close to ready, the dialog is close but need some CSS Component work, and there's still going to be a lot of work needed for the review request fields to tie them into the extensible field support we have.

  2. reviewboard/static/rb/css/common.less (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    We're trying to get rid of most of common.less and move to the CSS Component structure. This would be best in a rb/css/ui/shortcuts-dialog.less, as a .rb-c-shortcuts-dialog component, using effective style names.

    I know we're at the end of the term, but if you're interested in working on this after the semester ends, this would be a great task for that time! :) Otherwise consider it a reminder for us before it lands.

  3. Show all issues

    Can you wrap this in gettext() so the string can be localized?

  4. reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Looks like this would conflict with the other review request, right? That uses "docs," this uses "description."

    1. I haven't updated my other one (#10913) for some time. Do you think I should go back to update 10913? But since this one (10929) is branched out from 10913, there will be a conflict if I update 10913, right? Is this the right flow to do this: checkout to branch 10913 -> modify 10913 -> checkout branch 10929 -> rebase 10913 (branch 10929)

  5. Show all issues

    You can use $.ui.keyCode.TAB and 'H'.charCodeAt(0), respectively.

    It'd also be nice to be consistent with "KeyCode", rather than "Keycode".

  6. reviewboard/static/rb/js/pages/views/tests/basePageViewTests.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can make use of chaining here:

    pageView.$el
        .trigger(....)
        .trigger(....)
        ....;
    
  7. Show all issues

    It'd be nice to make these private methods (prefix with a _).

  8. Show all issues

    This should use _.isEmpty().

  9. Show all issues

    These must be localized. You'll want to pass in the localized strings as parameters to the template.

  10. Show all issues

    Right now, we should be using _.each() instead of Object.entries().forEach(). It has wider browser compatibility.

    We also cannot use fat arrow functions in templates. They'll be preserved and sent to the browser, and will break browsers without the support (not uncommon in enterprises).

    Basically, anything in a template skips Babel transpilation, so we can't use unsafe stuff.

    1. Thank you for pointing this out! I am having an issue with _.each():
      Suppose I have an object temp:

      'hH': {
          tabBasedCallback: '_showShortcutHelpDialog',
          description: gettext('Open shortcut help dialog'),
      }
      

      And when I do this:

      _.each(temp, function(row) {
          console.log(row);
      })
      

      will print {tabBasedCallback: "_showShortcutHelpDialog", description: "Open shortcut help dialog"}. This doesn't have the key hH which I need. Am I using this the wrong way?

  11. Show all issues

    You should use if (entry[1].tabBaseCallback). We want to test if it's present and has a value, and this is the safest, shortest way. (It will be undefined if missing.)

  12. Show all issues

    Same here.

  13. Show all issues

    We have to use var here, for browser compatibility.

  14. Show all issues

    Missing a semicolon.

  15. Show all issues

    This also needs to be localized.

  16. Show all issues

    This should be const (and appear as the first variable -- we do defined variables and consts before undefined and lets), since it won't be redefined.

  17. Show all issues

    I don't think we need to call .render() anymore. .show() does this for us, iirc.

  18. Show all issues

    We don't re-define this, so we should use const.

  19. Show all issues

    Comments should go on their own line and be proper sentences (capitalization, punctuation, "the", etc.).

  20. Show all issues

    Let's use _.size() here. It will guarantee the right thing for the browser it's running on.

  21. Show all issues

    Two things that can improve this:

    1. Let's pull out innerText into a variable, so we can check it without re-fetching it each time.
    2. Let's use expect(...).toContain('test1'), so we can get better output if the expectation fails.
  22. Show all issues

    Mind adding a comment about how this state is managed?

  23. Show all issues

    We should just reference $.ui.keyCode.TAB.

  24. Show all issues

    Instead of _.bind(), we should use =>.

    1. Are you saying something like:

      this.$el.on(`keydown.keybindings.${this.cid}`, evt => {
          if (evt.which === tabKeyCode) {
              isTabBasedShortcut = true;
          }
      });
      

      If I do this way, the code below will complain about calling func on undefined:
      func.call(this, evt);

  25. reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    You can chain these:

    this.$el
        .on(...)
        .on(...)
        ...;
    

    The binding below can also be included in the chain.

  26. reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    This is missing an "Args" section, documenting evt and isTabBasedShortcut.

  27. Show all issues

    Docstring summaries must be a single line.

    How about:

    /**
     * Call a shortcut handler when a key or combination is pressed.
    
  28. reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6)
     
     
     
     
     
     
     
    Show all issues

    This can be:

    const keyBinding = this.keyBindings[keys];
    
    let func = (isTabBasedShortcut
                ? keyBinding.tabBasedShortcut
                : keyBinding.callback);
    
  29. Show all issues

    Typo: "field"

  30. reviewboard/staticbundles.py (Diff revision 6)
     
     
    Show all issues

    Can you place this alphabetically in the list of test files?

  31. 
      
LittleGrey
Review request changed
Commits:
Summary ID Author
Add a keyboard shortcut central registry for page views.
20d4da8c4268f33c102486061da94fc8a66af18c Jay
Remove one extra space.
ff48beaa3d798a3d2fe07b612c7d561064db47f7 Jay
Add missing semicolon.
a324636d864dd8215f692e85425d4e2cef4d0ef4 Jay
Remove console.log and change some wording.
18847f629b301aeb2de567989234ed6690b0b73a Jay
Use extend instead of overwriting.
6e8c74bbf63591f72a84589f93add4a54a063020 Jay
Made keyBindings comments more clear.
a2a94100711811603e545a28e1d2ce5c37db92dd Jay
[WIP] Tab-based shortcuts on review request page fields.
1c9b40db935e92a8239a869cbbe14f5cf1388a10 Jay
Remove traling spaces.
538fe4586645ff2d69019858bd33e90c9edacf62 Jay
[WIP] Tab-based shortcuts on review request page.
a215d8037abd54564052af460459467f9c164ca6 Jay
Dynamically render keyboard shortcuts on each page
f0a4818f3dd6714626210881a0cf756f2873928a Jay
Better styles on shortcur help dialog.
025f1a15417988514668e3f4d1c8ffd5da988a74 Jay
Tab based shortcuts selecting fields & unit tests.
12de8777b68d977c3f5da45af4cac9eebe0487c2 Jay
Unit tests done.
9ee4a792a8e1659f08a965ef0ad488b07286acb4 Jay
Add a keyboard shortcut central registry for page views.
20d4da8c4268f33c102486061da94fc8a66af18c Jay
Remove one extra space.
ff48beaa3d798a3d2fe07b612c7d561064db47f7 Jay
Add missing semicolon.
a324636d864dd8215f692e85425d4e2cef4d0ef4 Jay
Remove console.log and change some wording.
18847f629b301aeb2de567989234ed6690b0b73a Jay
Use extend instead of overwriting.
6e8c74bbf63591f72a84589f93add4a54a063020 Jay
Made keyBindings comments more clear.
a2a94100711811603e545a28e1d2ce5c37db92dd Jay
[WIP] Tab-based shortcuts on review request page fields.
1c9b40db935e92a8239a869cbbe14f5cf1388a10 Jay
Remove traling spaces.
538fe4586645ff2d69019858bd33e90c9edacf62 Jay
[WIP] Tab-based shortcuts on review request page.
a215d8037abd54564052af460459467f9c164ca6 Jay
Dynamically render keyboard shortcuts on each page
f0a4818f3dd6714626210881a0cf756f2873928a Jay
Better styles on shortcur help dialog.
025f1a15417988514668e3f4d1c8ffd5da988a74 Jay
Tab based shortcuts selecting fields & unit tests.
12de8777b68d977c3f5da45af4cac9eebe0487c2 Jay
Unit tests done.
9ee4a792a8e1659f08a965ef0ad488b07286acb4 Jay
Address feedback - mostly improvements.
f2f0041bfb6468fad5189b7cf464855befd67196 Jay

Checks run (2 succeeded)

flake8 passed.
JSHint passed.