Make code line numbers work as links
Review Request #5576 — Created March 3, 2014 and submitted
Information | |
---|---|
Audore | |
Review Board | |
master | |
0ed8ba2... | |
Reviewers | |
students | |
mike_conley |
I added two functions to the file, one adds the links to the code line numbers and the another one removes the link when the code line number isn't hovered. The link is combined from the file id and the line number, which should be enough to identify the row, which should be highlighted. The link is removed from the th element, when user hovers on another row. I'm not sure if this is ideal solution, but otherwise the link keeps flicking every time the cursor moves even a bit.
The hackpad link -> (https://reviewboard.hackpad.com/Diffviewer-Line-Link-Project-Spec-jPwinbJva3E).
Testing is done on my development server. There is also two screen shots below to demonstrate, how the link is presented.
Description | From | Last Updated |
---|---|---|
Should the ghostflag show also here, when the code line link is generated? Though the ghostflag will show up, if … |
AU Audore | |
We use LessCSS, which allows you to nest rules in some convenient ways. This code can be rewritten as: a … |
|
|
Several issues with style that you'll need to fix up for all of your code: 1) Braces always go on … |
|
|
You should only ever have one var statement, and it should be at the very top of the function. In … |
|
|
You'll want to use this.getLineNum. You also have a lot of code in here doing things that aren't necessary. For … |
|
|
Looks into more of our functions and variables available in this class, like _getActualLineNumCell. You can reuse some of what … |
|
|
Make sure you're using single-quote strings and not double-quote wherever possible. |
|
|
Spaces around +. |
|
|
You're building this string in a couple places. I can't really tell why. Can you explain what that's doing? You … |
|
|
You need to be sure you're indenting when a statement spans multiple lines. Also, you should never need to call … |
|
|
Blank line before this. |
|
|
Careful about trailing whitespace. Go through your change and clean those up. |
|
|
These blank lines should go away. |
|
|
Similar comments as above. |
|
|
Blank line between these. |
|
|
I'm having trouble to implement this. This works fine if the diff is only one lined, but if there's two … |
AU Audore | |
You don't need to recurse like this. Just do: $row.find('.copylink') |
|
|
Get rid of these blank lines. |
|
|
I'm having trouble to implement this. This works fine if the diff is only one lined, but if there's two … |
AU Audore | |
Here I'm trying to declare the variables used in templates. |
AU Audore | |
Here i'm trying to bind the variables to the template. It's not working.. |
AU Audore | |
Here I try to use the template, so I woudn't pollute it with the same dom code over and over … |
AU Audore | |
I think your bug is that you're not passing linkToRow into this invocation of the template: ```javascript this._$link = $(this.linkTemplate({ … |
|
|
This isn't needed. |
|
|
I want to see this like other templates: <th> <a href .....> </th> |
|
|
These names are pretty generic and specific to the link code. Maybe oldLinkLineNum and oldLinkRow? |
|
|
Need a space after the comma. |
|
|
One variable per line. |
|
|
You still have an extra space before the === |
|
|
Blank line between these. |
|
|
Doing $myel[0].id is faster than $myel.attr('id'). |
|
|
Multi-line comments must be in the form of: /* * First line * Second line * ... */ Same with … |
|
|
You're querying 'th' all over the place, in multiple ways. Each time you query, it has to do some expensive … |
|
|
Same with the comment flags. |
|
|
As per my prior comments in other reviews, you don't need to be using .first() and .last() almost ever. Make … |
|
|
Indentation when chaining should be like: $row.children('th') .replaceWith(this.linkTemplate({ ... })); This applies all over the place. |
|
|
What's the difference between this and the ones above? Some extra, clear commenting would help a lot with the complexity … |
|
|
No blank line here. |
|
|
You have an extra space before the ) |
|
|
Blank line between these. |
|
|
We only want one var statement. |
|
|
Again, you're querying for stuff too many times. You need to condense these all down. |
|
|
This is already a <th>. You can just do: $row.find('th').text(lineNum); Same elsewhere. |
|
|
Don't add random blank lines in the file. You should go through and remove the unrelated code changes. |
|
|
Why did you do this? Those lines were very important. I can't imagine things are working without them. |
|
|
Please put a blank line between these two statements. |
|
|
Please put a blank line between these two statements. |
|
|
I think the alignment here will be easier to do cleanly if you break it into two statements. How about … |
|
|
There should be a space after the ":" in object literals. |
|
|
Typo in "exist". Also, please add a space after the '.' |
|
|
Please add a space after the '.' |
|
|
Can you put the })); on the next line? |
|
|
Can you separate these with a blank line? |
|
|
Can you put the })); on the next line? |
|
|
Same comments as in the previous method. |
|
|
Can you add a blank line here? |
|
|
After trying this out, I think that having the link show in underlined blue is very distracting, especially in cases … |
|
|
These two variables aren't used--you can get rid of them. |
|
|
In testing this, I noticed that mousing over the new link would de-highlight the row. The issue is that this … |
|
|
Because we're now expecting people to use the right button in the line number rows, this code should be wrapped … |
|
Change Summary:
Code line number link style added and some functionality fixed, only the ghost commentflag doesn't show up when hovering over code line number link.
Diff: |
Revision 2 (+63 -2) |
---|
-
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 3) We use LessCSS, which allows you to nest rules in some convenient ways. This code can be rewritten as:
a { color: black; text-decoration: none; &.copylink { ... } }
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Several issues with style that you'll need to fix up for all of your code:
1) Braces always go on the previous line, like:
if (...) {
2) Space after the
if
3) Be careful about extra spaces in the code (you have two before the
==
)4) In JavaScript, you almost always want to compare using
===
and!==
, not==
and!=
. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) You should only ever have one
var
statement, and it should be at the very top of the function. In JavaScript, you want to pre-declare all your variables (since internally, JavaScript will try to do that for you, which can lead to bugs). -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) You'll want to use
this.getLineNum
.You also have a lot of code in here doing things that aren't necessary. For example, you should never have to call
.filter(function() { ... })
for anything, and you likely don't need to call.first()
for anything here. Or.contents()
. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Looks into more of our functions and variables available in this class, like
_getActualLineNumCell
. You can reuse some of what we've already built.Also, note that you can call
.parents('table')
and similar to get a specific parent without having to go up each level manually. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Make sure you're using single-quote strings and not double-quote wherever possible.
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) You're building this string in a couple places. I can't really tell why. Can you explain what that's doing?
You also don't want to build this by hand here. Look at how we do templates in JavaScript (search for
_.template
). You'll want to create one for this and pass in the variables it'll use to build the entire string. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) You need to be sure you're indenting when a statement spans multiple lines.
Also, you should never need to call
.contents()
or.filter
. -
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Careful about trailing whitespace. Go through your change and clean those up.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) These blank lines should go away.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Similar comments as above.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Blank line between these.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) You don't need to recurse like this. Just do:
$row.find('.copylink')
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 3) Get rid of these blank lines.
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revisions 3 - 4) I'm having trouble to implement this. This works fine if the diff is only one lined, but if there's two lines and commentflags, getting the first/second (the one which isn't in tr attribute line) line number gets really difficult.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revisions 3 - 4) I'm having trouble to implement this. This works fine if the diff is only one lined, but if there's two lines and commentflags, getting the first/second (the one which isn't in tr attribute line) line number gets really difficult.
Change Summary:
Added the template code, which produces the following error:'Uncaught ReferenceError: linkToRow is not defined'
Diff: |
Revision 5 (+129 -4) |
---|
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 5) I think your bug is that you're not passing linkToRow into this invocation of the template:
```javascript
this._$link = $(this.linkTemplate({
linkToRow: ...
}));
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revisions 4 - 5) Here I'm trying to declare the variables used in templates.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revisions 4 - 5) Here i'm trying to bind the variables to the template. It's not working..
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revisions 4 - 5) Here I try to use the template, so I woudn't pollute it with the same dom code over and over again.
Change Summary:
Templating issue fixed and also the commenting flag issue fixed.
Diff: |
Revision 6 (+154 -6) |
---|
-
The biggest thing distracting me as I read this change is that your code is wildly inconsistent with the rest of the file, in terms of code style. When working on an existing codebase, it is very important that you make sure you are completely consistent. It should be impossible to tell what code you wrote on vs. what code was already there.
That includes things like:
- Number of spaces used for indentation (4)
- Make sure there's a space after
if
andfor
(noif()
) - Location of opening braces
- Location of ending braces
- One variable per line on
var
declarations - Line lengths (< 80 columns)
- Where blank lines go (also important not to randomly add or remove blank lines in unrelated parts of the file)
- Spaces after commas
Go through and make sure your code completely fits in with our code, and make sure that every single line you write from here on out also fits in. This is going to be an extremely important skill in the industry, where consistency is almost always required.
-
Nearly all the comments mentioned here apply in more than one place in the file. Make sure you go through in detail.
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) I want to see this like other templates:
<th> <a href .....> </th>
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) These names are pretty generic and specific to the link code. Maybe
oldLinkLineNum
andoldLinkRow
? -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Need a space after the comma.
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) You still have an extra space before the
===
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Blank line between these.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Doing
$myel[0].id
is faster than$myel.attr('id')
. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Multi-line comments must be in the form of:
/* * First line * Second line * ... */
Same with your other multi-line comments.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) You're querying 'th' all over the place, in multiple ways. Each time you query, it has to do some expensive lookups. Make sure you only query once per thing you're trying to get.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Same with the comment flags.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) As per my prior comments in other reviews, you don't need to be using
.first()
and.last()
almost ever. Make sure it's actually required when you use it. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Indentation when chaining should be like:
$row.children('th') .replaceWith(this.linkTemplate({ ... }));
This applies all over the place.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) What's the difference between this and the ones above?
Some extra, clear commenting would help a lot with the complexity of all this.
(I know English isn't your first language, but if you have any friends who are proficient, you can ask them to proof-read. Or come to us in IRC.)
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) You have an extra space before the
)
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Blank line between these.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) We only want one
var
statement. -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Again, you're querying for stuff too many times. You need to condense these all down.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) This is already a
<th>
. You can just do:$row.find('th').text(lineNum);
Same elsewhere.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Don't add random blank lines in the file. You should go through and remove the unrelated code changes.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 9) Why did you do this? Those lines were very important. I can't imagine things are working without them.
Change Summary:
Styling changes and refactoring the code
Commit: |
|
||
---|---|---|---|
Diff: |
Revision 10 (+127) |
Change Summary:
Spaces added, now every issue should be fixed.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+127) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+130) |
Change Summary:
Moved out of WIP, all issues cleared and two screen shots added.
Summary: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||
Testing Done: |
|
|||||||||||||||
Removed Files: |
||||||||||||||||
Added Files: |
-
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Please put a blank line between these two statements.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Please put a blank line between these two statements.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) I think the alignment here will be easier to do cleanly if you break it into two statements. How about this:
```javascript
this.$link = $(this.linkTemplate({
...
}));
this.$link
.on({
...
})
.hide()
.appendTo('body'); -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) There should be a space after the ":" in object literals.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Typo in "exist". Also, please add a space after the '.'
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Please add a space after the '.'
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Can you put the
}));
on the next line? -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Can you separate these with a blank line?
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Can you put the
}));
on the next line? -
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Same comments as in the previous method.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 12) Can you add a blank line here?
Change Summary:
Style changes added according the comments
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+137) |
-
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 13) After trying this out, I think that having the link show in underlined blue is very distracting, especially in cases where the event handling doesn't trigger the mouseout. I vote that we get rid of this and leave the appearance as-is.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 13) These two variables aren't used--you can get rid of them.
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 13) In testing this, I noticed that mousing over the new link would de-highlight the row. The issue is that this code isn't properly accounting for the new
<a>
element.If you add something like this at the beginning of this method, it should cope with it:
// Navigate up out of <a class="copylink"> elements if ($node.hasClass('copylink')) { $node = $node.parent(); } ...
-
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 13) Because we're now expecting people to use the right button in the line number rows, this code should be wrapped inside
if (e.button === 1) { ... }
Change Summary:
Removed the link styling and separated the right click and left click, what David suggested in his comments.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+142 -6) |