Summary:

Add a keyboard shortcut central registry for page views.

Review Request #10913 — Created Feb. 24, 2020 and updated April 8, 2020, 6:29 p.m.

Information
Owner:	LittleGrey
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Blocks:	10929
Reviewers
Groups:	reviewboard
People:

Description

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

Testing Done

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.");
},
...

Commits

Summary	Author
Add a keyboard shortcut central registry for page views.	Jay
Remove one extra space.	Jay
Add missing semicolon.	Jay
Remove console.log and change some wording.	Jay
Use extend instead of overwriting.	Jay
Made keyBindings comments more clear.	Jay

Screenshots

Issues

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 …	hxqlin	Feb. 27, 2020, 5:59 p.m.
Col: 61 Missing semicolon.	reviewbot	Feb. 24, 2020, 6:47 p.m.
Could change the wording from The dictionary to A dictionary	bui1	Feb. 24, 2020, 8:38 p.m.
use shortcut instead of shortcuts	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the previous comment. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the next file. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the previous diff. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the next diff. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the previous comment. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key selects the next comment. instead	bui1	Feb. 24, 2020, 8:38 p.m.
I'm not totally sure what this commands does on my first read through. I think changing the wording for this …	bui1	Feb. 24, 2020, 8:38 p.m.
Try This key creates a new comment. instead	bui1	Feb. 24, 2020, 8:38 p.m.
Could remove this console.log statement	bui1	Feb. 24, 2020, 8:38 p.m.
i think we usually use extend as opposed to expand	hxqlin	Feb. 27, 2020, 6:16 p.m.
not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since …	hxqlin	Feb. 27, 2020, 6:16 p.m.
i would prefer doc instead of help since it feels more specific but this is up to interpretation	hxqlin	Feb. 27, 2020, 6:16 p.m.
Nit: "help texts" -> "help text" We should also indicate what fields are expected in the dictionary. What is the …	mike_conley	March 1, 2020, 3:56 p.m.
I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this …	mike_conley	March 1, 2020, 3:56 p.m.
"&" -> "and"	chipx86	April 8, 2020, 6:27 p.m.
It would be great if this could document each key that's expected by an entry, using a format similar to …	chipx86	April 8, 2020, 6:27 p.m.
"consisting"	chipx86	April 8, 2020, 6:27 p.m.
For each of these, can you wrap the string with gettext()? That'll prepare it for localization.	chipx86	April 8, 2020, 6:27 p.m.
We should also leave off the "This key" part. Just like: "Select the previous file." "Select the next diff." "Re-center …	chipx86	April 8, 2020, 6:29 p.m.

flake8 passed.

JSHint failed.

JSHint

reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 1)
Show all issues
```
Col: 61
 Missing semicolon.
```

Commits:

	Summary	Author
-	Add a keyboard shortcut central registry for page views.	Jay
-	Remove one extra space.	Jay
+	Add a keyboard shortcut central registry for page views.	Jay
+	Remove one extra space.	Jay
+	Add missing semicolon.	Jay

Diff:

Revision 2 (+94 -22)

Show changes

	reviewboard/static/rb/js/pages/views/basePageView.es6.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/utils/keyBindingUtils.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 2)
Show all issues
```
Could change the wording from The dictionary to A dictionary
```
reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 2)
Show all issues
```
use shortcut instead of shortcuts
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the previous comment. instead
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the next file. instead
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the previous diff. instead
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the next diff. instead
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the previous comment. instead
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key selects the next comment. instead
```

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)

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

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2)
Show all issues
```
Try This key creates a new comment. instead
```
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 2)
Show all issues
```
Could remove this console.log statement
```

Commits:

	Summary	Author
-	Add a keyboard shortcut central registry for page views.	Jay
-	Remove one extra space.	Jay
-	Add missing semicolon.	Jay
+	Add a keyboard shortcut central registry for page views.	Jay
+	Remove one extra space.	Jay
+	Add missing semicolon.	Jay
+	Remove console.log and change some wording.	Jay

Diff:

Revision 3 (+102 -34)

Show changes

	reviewboard/static/rb/js/pages/views/basePageView.es6.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/utils/keyBindingUtils.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

LittleGrey Feb. 27, 2020, 6:16 p.m.

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.

reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 3)
Show all issues
```
i think we usually use extend as opposed to expand
```

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)

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

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)

Show all issues

i would prefer doc instead of help since it feels more specific but this is up to interpretation

Testing Done:

		The old keyboard shortcuts on `diffViewerPageView` are still working.

		Tested on Chrome and Firefox.

		Tested in `dataGridPageView` by adding this dummy code segment:

		`...`

~		keyBindings: {
	~	keyBindings: _.extend({
		'a': {
		callback: 'testAlert',
		}
~		},
	~	}, RB.PageView.prototype.keyBindings),

		testAlert() {
		alert("This is a test alert.");
		},
-
		...

Commits:

	Summary	Author
-	Add a keyboard shortcut central registry for page views.	Jay
-	Remove one extra space.	Jay
-	Add missing semicolon.	Jay
-	Remove console.log and change some wording.	Jay
+	Add a keyboard shortcut central registry for page views.	Jay
+	Remove one extra space.	Jay
+	Add missing semicolon.	Jay
+	Remove console.log and change some wording.	Jay
+	Use extend instead of overwriting.	Jay

Diff:

Revision 4 (+115 -47)

Show changes

	reviewboard/static/rb/js/pages/views/basePageView.es6.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/utils/keyBindingUtils.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Hi Xiaohui,

This looks like a great approach to me! Just a few minor suggestions.

reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 4)

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?

reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 4)

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

Commits:

	Summary	Author
-	Add a keyboard shortcut central registry for page views.	Jay
-	Remove one extra space.	Jay
-	Add missing semicolon.	Jay
-	Remove console.log and change some wording.	Jay
-	Use extend instead of overwriting.	Jay
+	Add a keyboard shortcut central registry for page views.	Jay
+	Remove one extra space.	Jay
+	Add missing semicolon.	Jay
+	Remove console.log and change some wording.	Jay
+	Use extend instead of overwriting.	Jay
+	Made keyBindings comments more clear.	Jay

Diff:

Revision 5 (+129 -49)

Show changes

	reviewboard/static/rb/js/pages/views/basePageView.es6.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/utils/keyBindingUtils.es6.js

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 5)
Show all issues
```
"&" -> "and"
```

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

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

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)

Show all issues

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

Fix it!

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5)

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.