Tab-based shortcuts on review request page fields.
Review Request #10929 — Created Feb. 29, 2020 and updated
Information | |
---|---|
LittleGrey | |
Review Board | |
master | |
10913 | |
Reviewers | |
reviewboard | |
This review request is for the 2nd part of "selecting review request
field" project. The goal of this project is to havetab
-based keybaord
shortcuts on review request page fields. (e.g., hittingtab-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
andh
on review request page and the dialog is shown (the
shortcuts are displayed as well).Press
tab
andd
on review request page and Description field is
focused.Press
tab
andh
on dashboard page and the help dialog is shown.All unit tests passed.
Summary | Author |
---|---|
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
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 … |
|
|
Can you wrap this in gettext() so the string can be localized? |
|
|
Looks like this would conflict with the other review request, right? That uses "docs," this uses "description." |
|
|
You can use $.ui.keyCode.TAB and 'H'.charCodeAt(0), respectively. It'd also be nice to be consistent with "KeyCode", rather than "Keycode". |
|
|
You can make use of chaining here: pageView.$el .trigger(....) .trigger(....) ....; |
|
|
It'd be nice to make these private methods (prefix with a _). |
|
|
This should use _.isEmpty(). |
|
|
These must be localized. You'll want to pass in the localized strings as parameters to the template. |
|
|
Right now, we should be using _.each() instead of Object.entries().forEach(). It has wider browser compatibility. We also cannot use fat … |
|
|
You should use if (entry[1].tabBaseCallback). We want to test if it's present and has a value, and this is the … |
|
|
Same here. |
|
|
We have to use var here, for browser compatibility. |
|
|
Missing a semicolon. |
|
|
This also needs to be localized. |
|
|
This should be const (and appear as the first variable -- we do defined variables and consts before undefined and … |
|
|
I don't think we need to call .render() anymore. .show() does this for us, iirc. |
|
|
We don't re-define this, so we should use const. |
|
|
Comments should go on their own line and be proper sentences (capitalization, punctuation, "the", etc.). |
|
|
Let's use _.size() here. It will guarantee the right thing for the browser it's running on. |
|
|
Two things that can improve this: Let's pull out innerText into a variable, so we can check it without re-fetching … |
|
|
Mind adding a comment about how this state is managed? |
|
|
We should just reference $.ui.keyCode.TAB. |
|
|
Instead of _.bind(), we should use =>. |
|
|
You can chain these: this.$el .on(...) .on(...) ...; The binding below can also be included in the chain. |
|
|
This is missing an "Args" section, documenting evt and isTabBasedShortcut. |
|
|
Docstring summaries must be a single line. How about: /** * Call a shortcut handler when a key or combination … |
|
|
This can be: const keyBinding = this.keyBindings[keys]; let func = (isTabBasedShortcut ? keyBinding.tabBasedShortcut : keyBinding.callback); |
|
|
Typo: "field" |
|
|
Can you place this alphabetically in the list of test files? |
|

Change Summary:
More changes,
tab-h
can trigger help dialog now.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+361 -83) |
Checks run (2 succeeded)

Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+561 -181) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Checks run (2 succeeded)

Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+620 -190) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Removed Files: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
Checks run (2 succeeded)

Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+782 -206) |
Checks run (2 succeeded)

Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 6 (+905 -233) |
Checks run (2 succeeded)
-
-
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 arb/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.
-
reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 6) Can you wrap this in
gettext()
so the string can be localized? -
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."
-
reviewboard/static/rb/js/pages/views/tests/basePageViewTests.es6.js (Diff revision 6) You can use
$.ui.keyCode.TAB
and'H'.charCodeAt(0)
, respectively.It'd also be nice to be consistent with "KeyCode", rather than "Keycode".
-
reviewboard/static/rb/js/pages/views/tests/basePageViewTests.es6.js (Diff revision 6) You can make use of chaining here:
pageView.$el .trigger(....) .trigger(....) ....;
-
-
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) This should use
_.isEmpty()
. -
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) These must be localized. You'll want to pass in the localized strings as parameters to the template.
-
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) Right now, we should be using
_.each()
instead ofObject.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.
-
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) 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 beundefined
if missing.) -
-
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) We have to use
var
here, for browser compatibility. -
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) Missing a semicolon.
-
reviewboard/static/rb/js/ui/views/shortcutHelpDialogView.es6.js (Diff revision 6) This also needs to be localized.
-
This should be
const
(and appear as the first variable -- we do defined variables andconst
s before undefined andlet
s), since it won't be redefined. -
-
-
Comments should go on their own line and be proper sentences (capitalization, punctuation, "the", etc.).
-
-
Two things that can improve this:
- Let's pull out
innerText
into a variable, so we can check it without re-fetching it each time. - Let's use
expect(...).toContain('test1')
, so we can get better output if the expectation fails.
- Let's pull out
-
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6) Mind adding a comment about how this state is managed?
-
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6) We should just reference
$.ui.keyCode.TAB
. -
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6) Instead of
_.bind()
, we should use=>
. -
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.
-
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6) This is missing an "Args" section, documenting
evt
andisTabBasedShortcut
. -
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 6) Docstring summaries must be a single line.
How about:
/** * Call a shortcut handler when a key or combination is pressed.
-
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);
-
-
reviewboard/staticbundles.py (Diff revision 6) Can you place this alphabetically in the list of test files?

Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+1035 -317) |