- Status:
- Completed
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:
-
Added code to detect key pressesCreated a keyboard shortcut to add comments to a diff
- Description:
-
Fixes issue #44
~ Created a keyboard shortcut (r, R, C - c was already taken) to add comments to a block of a diff.
~ Created a keyboard shortcut (r, R, C - c was already taken) 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.
-
-
The last item in javascript objects or arrays shouldn't have a comma after it (IE will fail to parse it).
-
-
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.
-
-
-
-
-
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 = ... -
-
-
ED
- Description:
-
Fixes issue #44
~ Created a keyboard shortcut (r, R, C - c was already taken) 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.
~ 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.
ED
- Testing Done:
-
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.