Created a keyboard shortcut to add comments to a diff
Review Request #5442 — Created Feb. 8, 2014 and submitted
Fixes issue #44
Created a keyboard shortcut (r, R) to add comments to a block of a diff. Also saved the chunk that is highlighted when navigating the diff using keyboard shortcuts to use for detecting where to put the comments.
Navigated through a diff using keyboard shortcuts and pressed all three of the keyboard shortcuts to bring up a comment, which bring up the comment box.
If no block is selected, nothing happens when the shortcuts are pressed.
Tested on multi and single line blocks.
Tested on a diff with two files.
| Description | From | Last Updated |
|---|---|---|
|
The last item in javascript objects or arrays shouldn't have a comma after it (IE will fail to parse it). |
 david
|
|
|
Please remove "This" (have it just start with "Creates ...") and end the sentence with a period. |
david
|
|
|
Please wrap this line to 80 characters. You should be able to just put the "endNode" on the next line, … |
david
|
|
|
Last item shouldn't have a comma after it. |
david
|
|
|
_chunk is kind of an ambiguous name. How about _highlightedChunk? |
david
|
|
|
We should reset this._chunk here, since the files are getting reloaded/changed. |
david
|
|
|
Add a space between the () and the {. |
david
|
|
|
All of these should be prefixed with var. We also try to have a single var statement, to fit in … |
david
|
|
|
var definitions should be the first statement in the function. |
david
|
|
|
Please wrap this to 80 columns. |
david
|
|
|
Same comment as the similar code in diffReviewableView.js. |
david
|
|
|
Hmm. How about just using 'c' instead of both 'r' and 'c'? |
david
|
|
|
The lines starting with '.' should be indented 4 spaces instead of 8. |
david
|
|
|
The line starting with '.' should be indented 4 spaces instead of 8. |
david
|
|
|
Can you capitalize the C in "chunk" for this variable name? |
david
|
|
|
Hmm. I just realized that this is calling createComment for every file in the diff, which doesn't seem right. Did … |
david
|
ED
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
-
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) The last item in javascript objects or arrays shouldn't have a comma after it (IE will fail to parse it).
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) Please remove "This" (have it just start with "Creates ...") and end the sentence with a period.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) Please wrap this line to 80 characters. You should be able to just put the "endNode" on the next line, indented to line up with "beginLineNum" above it.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Last item shouldn't have a comma after it.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) _chunkis kind of an ambiguous name. How about_highlightedChunk? -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) We should reset
this._chunkhere, since the files are getting reloaded/changed. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Add a space between the
()and the{. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) All of these should be prefixed with
var. We also try to have a singlevarstatement, to fit in with linting tools like jshint.We also should make sure that lines wrap to 80 columns.
So this should look something like:
var chunkID = this._chunk[0].id, lineElements = document .getElementById(chunkId) .getElementsByTagName('tr'), beginLineNum = ... -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) vardefinitions should be the first statement in the function. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Please wrap this to 80 columns.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 1) Same comment as the similar code in diffReviewableView.js.
-
Can you go through the old comments and click "Fixed" on all the opened issues that you've fixed?
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2) Hmm. How about just using 'c' instead of both 'r' and 'c'?
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2) The lines starting with '.' should be indented 4 spaces instead of 8.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2) The line starting with '.' should be indented 4 spaces instead of 8.
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) Can you capitalize the C in "chunk" for this variable name?
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 5) Hmm. I just realized that this is calling createComment for every file in the diff, which doesn't seem right. Did you try this with a diff that contained multiple files?
Perhaps we should do something like conditionalize this on
$.contains(diffReviewableView.el, beginNode)?
ED
Description: |
|
|---|
ED
Testing Done: |
|
|---|