Highlighting the row, which is in the URL
Review Request #5706 — Created April 12, 2014 and submitted
Information | |
---|---|
Audore | |
Review Board | |
master | |
843fcf7... | |
Reviewers | |
reviewboard, students | |
Highlighting the row, which is in the URL. This is the part 2 of my project. The goal is to highlight the row, which is in the URL (example:
localhost:8080/r/1/diff/#file2,1072) based on the file and the line number.
Testing done on my development server locally, works fine there
Description | From | Last Updated |
---|---|---|
It would be nice to combine this a bit with the selector passed into diffReviewableView. How about this in the … |
|
|
This could also be defined in the initial variable definition instead of as a separate statement. |
|
|
Spacing is a bit weird here. Add a space between if and (, and remove one of the two after … |
|
|
Please add spaces around the + characters. |
|
|
Put the })); on the next line. |
|
|
You should probably merge the code above with this--we don't want to add a second anchor if one already exists, … |
|
|
You use this hex color in two places--mind defining a variable for it in defs.less? Also, add a space after … |
|
|
This should use a variable for the color, and include a space after the : |
|
|
This should use a variable for the color, and include a space after the : |
|
|
This should use a variable for the color, and include a space after the : |
|
|
This is closer, but still doesn't do quite the right thing for the case of comment anchors which are added … |
|
|
There are a ton of typos in this comment. |
|
|
This change exposed an existing bug in the code. If the URL contains any hash, we want to strip it … |
|
|
If this._startAtAnchorName is null, this line will fail (and break the diff viewer). Can you change this line to just … |
|
-
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) It would be nice to combine this a bit with the selector passed into diffReviewableView. How about this in the variable definitions:
var elementName = 'file' + diffReviewable.get('fileDiffID'), diffReviewableView = new RB.DiffReviewableView({ el: $('#' + elementName'), model: diffReviewable }), ...
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) This could also be defined in the initial variable definition instead of as a separate statement.
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Spacing is a bit weird here. Add a space between
if
and(
, and remove one of the two after&&
. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Please add spaces around the
+
characters. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) Put the
}));
on the next line. -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 1) You should probably merge the code above with this--we don't want to add a second anchor if one already exists, and it would be nice to keep these things next to each other.
Change Summary:
Adding the highlight color, refacotring the code and moving out of WIP
Summary: |
|
||||
---|---|---|---|---|---|
Testing Done: |
|
||||
Commit: |
|
||||
Diff: |
Revision 2 (+34 -1) |
Change Summary:
ElementName fixed
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+36 -3) |
-
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 3) You use this hex color in two places--mind defining a variable for it in defs.less?
Also, add a space after the :
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 3) This should use a variable for the color, and include a space after the :
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 3) This should use a variable for the color, and include a space after the :
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 3) This should use a variable for the color, and include a space after the :
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) This is closer, but still doesn't do quite the right thing for the case of comment anchors which are added by the template. I think probably what you need is something more like the following. Note that I haven't tested it, it's just a proposal for how to structure this code.
if (this._startAtAnchorName) { $anchor = $('a[name]"' + this._startAtAnchorName + '"]'); /* * Some anchors are added by the template (such as those at * comment locations), but not all are. If the anchor isn't found, * but the URL hash is indicating that we want to start at a * location within this file, add the anchor. * / if ($anchor.length === 0 && urlSplit.length === 2 && elementName === urlSplit[0]) { $anchor = $(this.anchorTemplate({ anchorName: this._startAtAnchorName }); diffReviewableView.$el .find("tr[line='" + urlSplit[1] + "']) .addClass('highlight-anchor') .append($anchor); } if ($anchor.length !== 0) { this.selectAnchor($anchor); this._startAtAnchorName = null; } }
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 3) There are a ton of typos in this comment.
Change Summary:
Fixed the stylesheet issues and refactored the code according to the comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+41 -3) |
-
Just a couple small changes left!
-
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 4) This change exposed an existing bug in the code. If the URL contains any hash, we want to strip it out here (otherwise if you copy a URL from a page which was navigated to you might end up with something like "diff/1/file1672,3#file1672,4" which doesn't work. Can you change this to just be
if (window.location.hash)
? -
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 4) If
this._startAtAnchorName
is null, this line will fail (and break the diff viewer). Can you change this line to just define the variable, and then assign it within theif (this._startAtAnchorName)
block?
Change Summary:
FIxed the small issues
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+44 -5) |