1286027 - Display ReviewRequestSummary diffstat information in MozReview section

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Description

[Steven MacLeod [:smacleod]]

We should display the diffstat information from Bug 1286017 in the MozReview request section on BMO.

Comment 1

[Mark Côté [:mcote]]

Attachment: Screenshot, added lines only

Here's a screenshot of the MozReview Requests table, adding in a "Changes" column.  This particular one shows only line additions ("+3").  I'll upload another with both additions & deletions.

At glob's suggestion, I also removed the commit hash and made the summary link to the diff view instead of the review view, since presumably most people coming from BMO will be more interested in the diff.

Comment 2

[Mark Côté [:mcote]]

Attachment: Screenshot, added and removed lines

Same as before, but with both added and removed lines.

Comment 3

[Mark Côté [:mcote]]

Attachment: Patch, v1

BMO patch.  I'll wait until people take a look at the screenshots before asking for review.

Comment 4

[:glob ✱]

Comment on attachment 8773765 [details] [diff] [review]
Patch, v1

Review of attachment 8773765 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob lgtm

::: extensions/MozReview/web/style/mozreview.css
@@ +50,5 @@
>      text-align: center;
>  }
>  
> +.mozreview-requests .mozreview-diffstat {
> +    text-align: center;

probably should nowrap this to ensure a long commit desc wraps instead of this column.

Comment 5

[Mark Côté [:mcote]]

Attachment: Screenshot, lots of big changes, use "k" for > 1000 lines changed

As requested, here's what it looks like with big changes.  I also did the same as davidwalsh and used "k" when > 1000 lines changed.

Comment 6

[Mark Côté [:mcote]]

Comment on attachment 8774956 [details]
Screenshot, lots of big changes, use "k" for > 1000 lines changed

I'm debating whether the last three column headers should be centred or not.  Or maybe just "Changes".

Comment 7

[Mark Côté [:mcote]]

Attachment: Patch, v2

Updated patch, with humanized integers and "white-space: nowrap" at glob's suggestion.

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8774956 [details]
Screenshot, lots of big changes, use "k" for > 1000 lines changed

The changes column being centered feels really strange to me... I think I'd prefer left aligned? Or maybe what I really want is two columns... This is all pretty wishy washy, so feel free to ignore me.

Comment 9

[:glob ✱]

Comment on attachment 8774957 [details] [diff] [review]
Patch, v2

Review of attachment 8774957 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/MozReview/web/js/mozreview.js
@@ +28,5 @@
>      }
>  
> +    function humanizedInt(i) {
> +        if (i > 1000) {
> +            return Math.floor(i/1000) + 'k';

this isn't very accurate.
eg. if a patch has 9999 lines it will be reported as "9k".

how about something like this instead:
+((i/1000).toFixed(1)) + ''

@@ +33,5 @@
> +        } else {
> +            return '' + i;
> +        }
> +    }
> + 

remove trailing whitespace

Comment 10

[Mark Côté [:mcote]]

Attachment: Patch, v3

Changes made as suggested.

Note that the Commits table in Review Board currently does what v2 of this patch did.  I'll file a bug for that.

Comment 11

[Mark Côté [:mcote]]

Filed bug 1291065 for the corresponding Review Board change

Comment 12

[Mark Côté [:mcote]]

Attachment: Patch, v4

.mozreview-diff-link is no longer used.

Comment 13

[:glob ✱]

Comment on attachment 8776803 [details] [diff] [review]
Patch, v4

Review of attachment 8776803 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/MozReview/web/js/mozreview.js
@@ +28,5 @@
>      }
>  
> +    function humanizedInt(i) {
> +        if (i > 1000) {
> +            return ((i/1000).toFixed(1)) + 'k';

i generally prefer to not display .0 (ie. output "1k" not "1.0k"), but this is fine as is.

nit: you don't need the outer set of parentheses, and there should be spaces around operators:
return (i / 1000).toFixed(1) + 'k';

Comment 14

[Mark Côté [:mcote]]

To [e-mail]:mozilla-bteam/bmo.git
   04eaf29..df005ea  master -> master