-
-
reviewboard/static/rb/js/diffviewer.js (Diff revision 1) Why not just assign RB.scrollToAnchor above and update things in this file to use that?
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) It'll be shorter if you put all args on the second line.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) There's a lot of explicit null checking for this variable, where I think truthiness works just as well, and would be more readable. Others too, but _$begin seems to be the worst offender.
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) When conditionals line up like this, I find it incredibly hard to tell where the conditional ends and the block begins. How would you feel about either using the && at the beginning of the second line of the conditional, or putting a blank line at the top of the block?
-
reviewboard/templates/diffviewer/view_diff.html (Diff revision 1) Why did you change this? If it's no longer necessary, can you remove its definition in reviews/views.py?
Move line commenting and chunk selection functionality into DiffReviewerView.
Review Request #4248 — Created June 18, 2013 and submitted
Move line commenting and chunk selection functionality into DiffReviewerView. The majority of code that lived in $.fn.diffFile has been moved into the new reviewable. All the code that handles line selection is in a CommentDiffSelector object, which listens for events on the diff reviewable's element. This keeps the main reviewable much cleaner and easier to work with. This also brings back the ghost comment flag, which disappeared in 1.7 (due to a CSS rule change). That'll probably get backported to 1.7.
Tested commenting on single lines. Tested commenting across multiple lines. Tested clicking on comments. Tested expanding comments. Tested clicking on chunks, causing highlights. Tested anchors to lines in the diff viewer. Saw the ghost flag.
| Description | From | Last Updated |
|---|---|---|
|
Why not just assign RB.scrollToAnchor above and update things in this file to use that? |
 david
|
|
|
It'll be shorter if you put all args on the second line. |
david
|
|
|
There's a lot of explicit null checking for this variable, where I think truthiness works just as well, and would … |
david
|
|
|
When conditionals line up like this, I find it incredibly hard to tell where the conditional ends and the block … |
david
|
|
|
Why did you change this? If it's no longer necessary, can you remove its definition in reviews/views.py? |
david
|
Change Summary:
* Removed some unnecessary comparisons against null. * Removed specific_diff_requested. * Fixed a hard-to-read conditional alignment. * Moved DiffReviewable unit tests and fixed them up to work with the new changes to getRenderedDiff.