Tab-based shortcuts on review request page fields.

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

LittleGrey
Review Board
master
10913
reviewboard

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 Author
Better styles on shortcur help dialog.
Jay
Tab based shortcuts selecting fields & unit tests.
Jay
Remove console.log and change some wording.
Jay
[WIP] Tab-based shortcuts on review request page fields.
Jay
Add a keyboard shortcut central registry for page views.
Jay
Remove traling spaces.
Jay
Use extend instead of overwriting.
Jay
Unit tests done.
Jay
[WIP] Tab-based shortcuts on review request page.
Jay
Made keyBindings comments more clear.
Jay
Add missing semicolon.
Jay
Dynamically render keyboard shortcuts on each page
Jay
Address feedback - mostly improvements.
Jay
Remove one extra  space.
Jay
Loading file attachments...

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)
     
     
     
     
     
     

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

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

    You can make use of chaining here:

    pageView.$el
        .trigger(....)
        .trigger(....)
        ....;
    
  7. It'd be nice to make these private methods (prefix with a _).

  8. This should use _.isEmpty().

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

  10. 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. 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. We have to use var here, for browser compatibility.

  13. This also needs to be localized.

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

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

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

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

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

  19. 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.
  20. Mind adding a comment about how this state is managed?

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

  22. 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);

  23. reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6)
     
     
     
     
     
     
     
     

    You can chain these:

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

    The binding below can also be included in the chain.

  24. reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6)
     
     
     
     
     
     
     

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

  25. Docstring summaries must be a single line.

    How about:

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

    This can be:

    const keyBinding = this.keyBindings[keys];
    
    let func = (isTabBasedShortcut
                ? keyBinding.tabBasedShortcut
                : keyBinding.callback);
    
  27. reviewboard/staticbundles.py (Diff revision 6)
     
     

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

  28. 
      
LittleGrey
Review request changed

Commits:

Summary Author
-
Better styles on shortcur help dialog.
Jay
-
Tab based shortcuts selecting fields & unit tests.
Jay
-
Remove console.log and change some wording.
Jay
-
[WIP] Tab-based shortcuts on review request page fields.
Jay
-
Add a keyboard shortcut central registry for page views.
Jay
-
Remove traling spaces.
Jay
-
Use extend instead of overwriting.
Jay
-
Unit tests done.
Jay
-
[WIP] Tab-based shortcuts on review request page.
Jay
-
Made keyBindings comments more clear.
Jay
-
Add missing semicolon.
Jay
-
Dynamically render keyboard shortcuts on each page
Jay
-
Remove one extra  space.
Jay
+
Better styles on shortcur help dialog.
Jay
+
Tab based shortcuts selecting fields & unit tests.
Jay
+
Remove console.log and change some wording.
Jay
+
[WIP] Tab-based shortcuts on review request page fields.
Jay
+
Add a keyboard shortcut central registry for page views.
Jay
+
Remove traling spaces.
Jay
+
Use extend instead of overwriting.
Jay
+
Unit tests done.
Jay
+
[WIP] Tab-based shortcuts on review request page.
Jay
+
Made keyBindings comments more clear.
Jay
+
Add missing semicolon.
Jay
+
Dynamically render keyboard shortcuts on each page
Jay
+
Address feedback - mostly improvements.
Jay
+
Remove one extra  space.
Jay

Diff:

Revision 7 (+1035 -317)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...