Summary

Added tooltips for the graphs that represent diff change summaries in the diff viewer.

Review Request #7880 — Created Jan. 16, 2016 and submitted Feb. 4, 2016, 5:32 p.m.

Information

Owner

evanhunzinger

Repository

Review Board

Branch

release-2.0.x

Bugs

3812

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Right now, the diff viewer shows graphs beside each modified file to show whether or not files were added/deleted, how many lines were inserted/changed/deleted, or if there was an error loading the diff information for a given file.

Users were able to depict what the graphs typically mean, however it was desired that tooltips were added for each of the icons to give a detailed description of the change summary for that file.

Testing Done

In my local environment I posted reviews with every type of modification to a file (as well as an erroneous diff of a file) to ensure tooltip was both correct and displaying properly.

Issues

Description	From	Last Updated
We already query this element once. We don't want to do it again, so let's pull it out into a …	chipx86	Jan. 17, 2016, 2:54 p.m.
Make sure all new variables are declared with var at the top. Also, we only ever want one var statement …	chipx86	Jan. 17, 2016, 2:54 p.m.
We should wrap this for translation, and use single quotes: tooltip = gettext('New file');	david	Jan. 17, 2016, 2:53 p.m.
We should wrap this for translation, and use single quotes: tooltip = gettext('Deleted file');	david	Jan. 17, 2016, 2:53 p.m.
There's not a ton of reason to have this be an "else if" -- we can just have this be …	david	Jan. 17, 2016, 2:53 p.m.
The translation utilities provide nice pluralization and formatting capabilities: tooltip += interpolate( ngettext('%s new line', '%s new lines', numInserts), [numInserts]) …	david	Jan. 17, 2016, 2:54 p.m.
There should be blank lines between blocks (like if statements, for loops, etc.) and other code. So, blank line here, …	chipx86	Jan. 17, 2016, 2:54 p.m.
Alignment errors.	chipx86	Jan. 17, 2016, 2:55 p.m.
Since we're using jquery, we can use the .attr method: $item.find('.diff-file-icon').attr('title', tooltip); We probably also don't need to gate it …	david	Jan. 17, 2016, 2:54 p.m.
We're querying the diff icon twice here as well. These sort of lookups are expensive, and we want to be …	chipx86	Jan. 17, 2016, 2:55 p.m.
We should call this variable $fileIcon to indicate that it's a jquery.	david	Jan. 28, 2016, 12:57 a.m.
Trailing whitespace, and since we're indenting the second argument, let's put the first on its own line too: $fileIcon.attr( 'title', …	david	Jan. 28, 2016, 12:57 a.m.
Call this $fileIcon.	david	Jan. 28, 2016, 12:57 a.m.
How about tooltipParts instead of mods?	david	Jan. 28, 2016, 12:57 a.m.
Technically this should be %d instead of %s.	brennie	Jan. 28, 2016, 7:20 p.m.
%d	brennie	Jan. 28, 2016, 7:20 p.m.
%d	brennie	Jan. 28, 2016, 7:20 p.m.
Trailing whitespace.	david	Jan. 28, 2016, 12:57 a.m.
Trailing whitespace.	david	Jan. 28, 2016, 12:57 a.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

We should wrap this for translation, and use single quotes:

tooltip = gettext('New file');

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

We should wrap this for translation, and use single quotes:

tooltip = gettext('Deleted file');

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

There's not a ton of reason to have this be an "else if" -- we can just have this be an "else" clause and let the inner conditionals do this work.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

The translation utilities provide nice pluralization and formatting capabilities:

tooltip += interpolate(
    ngettext('%s new line', '%s new lines', numInserts),
    [numInserts])


It might also be nice to do this by creating an array of parts and using ', '.join(parts).
There's also some trailing whitespace introduced in here.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

Since we're using jquery, we can use the .attr method:

$item.find('.diff-file-icon').attr('title', tooltip);

We probably also don't need to gate it in the if statement because if the tooltip is empty, we'll just set the title to an empty string.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

We already query this element once. We don't want to do it again, so let's pull it out into a variable that both statements can use.

We also need to localize the text by calling gettext('...').

Along with this, we want more user-friendly error messages. "There was an error loading the diff. See the details below." sort of a thing.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

Make sure all new variables are declared with var at the top. Also, we only ever want one var statement per function. See how we do it in other blocks of code.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

There should be blank lines between blocks (like if statements, for loops, etc.) and other code. So, blank line here, and same with ones below.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

Alignment errors.

evanhunzinger Jan. 16, 2016, 8:31 p.m.

I'm not sure what you mean, do you mean that 'numReplaces + " line"' should be on a new line, or did I do the new line improperly with '((numReplaces > 1)...'?

evanhunzinger Jan. 17, 2016, 2:55 p.m.

After quick discussion with David, the formatting issues were fixed with the new solution.

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 1)

The issue has been resolved. Show all issues

We're querying the diff icon twice here as well. These sort of lookups are expensive, and we want to be as careful as possible to not do them more than once.

Also, please put the bug number into the "bugs" field.

Groups:: students

Bugs:: 3812

Change Summary:

Reduced DOM tree traversals by putting the element in a variable, utilized localization method for strings, declared variables properly, and cleaned up the generation of tooltip message.

Diff:

Revision 2 (+40 -3)

Show changes

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
We should call this variable $fileIcon to indicate that it's a jquery.
```

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)

The issue has been resolved. Show all issues

Trailing whitespace, and since we're indenting the second argument, let's put the first on its own line too:

$fileIcon.attr(
    'title',
    gettext('There was an error loading this diff. See the details below'));

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Call this $fileIcon.
```
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
How about tooltipParts instead of mods?
```
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)

The issue has been dropped. Show all issues

Technically this should be %d instead of %s.

evanhunzinger Jan. 28, 2016, 12:44 a.m.

After some testing and googling I don't think ngettext() supports anything but %s.

david Jan. 28, 2016, 12:46 a.m.

Yeah, this is javascript, and the gettext implementations there are pretty minimal.

EV

evanhunzinger Jan. 28, 2016, 12:50 a.m.
```
So keep it the way it is?
```
david Jan. 28, 2016, 2:04 a.m.
```
Yep. Feel free to drop the issue(s).
```

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been dropped. Show all issues
```
%d
```
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 2)
The issue has been dropped. Show all issues
```
%d
```

Diff:

Revision 3 (+41 -3)

Show changes

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (ceb2b88)