JSHint
-
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 1) Show all issues
Review Request #10913 — Created Feb. 24, 2020 and updated
Information | |
---|---|
LittleGrey | |
Review Board | |
master | |
10929 | |
Reviewers | |
reviewboard | |
Created an object which can be a central registry for keyboard
shortcuts on the current page. This new objectkeyBindings
is
owned byRB.Pageview
. Subclasses ofRB.PageView
(e.g.,diffViewerPageView
anddataGridPageView
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 andhelp
(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 | Author |
---|---|
Jay | |
Jay | |
Jay | |
Jay | |
Jay | |
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 … |
|
|
Col: 61 Missing semicolon. |
![]() |
|
Could change the wording from The dictionary to A dictionary |
![]() |
|
use shortcut instead of shortcuts |
![]() |
|
Try This key selects the previous comment. instead |
![]() |
|
Try This key selects the next file. instead |
![]() |
|
Try This key selects the previous diff. instead |
![]() |
|
Try This key selects the next diff. instead |
![]() |
|
Try This key selects the previous comment. instead |
![]() |
|
Try This key selects the next comment. instead |
![]() |
|
I'm not totally sure what this commands does on my first read through. I think changing the wording for this … |
![]() |
|
Try This key creates a new comment. instead |
![]() |
|
Could remove this console.log statement |
![]() |
|
i think we usually use extend as opposed to expand |
|
|
not sure but i think defining keyBindings like this will overwrite the parent object; it's not a problem atm since … |
|
|
i would prefer doc instead of help since it feels more specific but this is up to interpretation |
|
|
Nit: "help texts" -> "help text" We should also indicate what fields are expected in the dictionary. What is the … |
|
|
I wonder if we should be more explicit in encouraging subclasses to extend this, by saying "Subclasses should extend this … |
|
|
"&" -> "and" |
|
|
It would be great if this could document each key that's expected by an entry, using a format similar to … |
|
|
"consisting" |
|
|
For each of these, can you wrap the string with gettext()? That'll prepare it for localization. |
|
|
We should also leave off the "This key" part. Just like: "Select the previous file." "Select the next diff." "Re-center … |
|
Commits: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+94 -22) |
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) |
---|
Could change the wording from
The dictionary
toA dictionary
reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 2) |
---|
use
shortcut
instead ofshortcuts
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the previous comment.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the next file.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the previous diff.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the next diff.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the previous comment.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
Try
This key selects the next comment.
instead
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 2) |
---|
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) |
---|
Try
This key creates a new comment.
instead
reviewboard/static/rb/js/utils/keyBindingUtils.es6.js (Diff revision 2) |
---|
Could remove this console.log statement
Commits: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+102 -34) |
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.
reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 3) |
---|
i think we usually use
extend
as opposed toexpand
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3) |
---|
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) |
---|
i would prefer
doc
instead ofhelp
since it feels more specific but this is up to interpretation
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+115 -47) |
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) |
---|
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) |
---|
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: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+129 -49) |
reviewboard/static/rb/js/pages/views/basePageView.es6.js (Diff revision 5) |
---|
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/diffViewerPageView.es6.js (Diff revision 5) |
---|
For each of these, can you wrap the string with
gettext()
? That'll prepare it for localization.
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 5) |
---|
We should also leave off the "This key" part. Just like:
etc.