Closed Bug 1286027 Opened 4 years ago Closed 3 years ago

Display ReviewRequestSummary diffstat information in MozReview section

Categories

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

Production
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smacleod, Unassigned)

References

Details

Attachments

(4 files, 3 obsolete files)

We should display the diffstat information from Bug 1286017 in the MozReview request section on BMO.
Component: Integration: Bugzilla → Extensions: MozReview Integration
Product: MozReview → bugzilla.mozilla.org
Version: unspecified → Production
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.
Attachment #8773762 - Flags: review?(smacleod)
Attachment #8773762 - Flags: review?(glob)
Attachment #8773762 - Flags: review?(dwalsh)
Same as before, but with both added and removed lines.
Attachment #8773763 - Flags: review?(smacleod)
Attachment #8773763 - Flags: review?(glob)
Attachment #8773763 - Flags: review?(dwalsh)
Attached patch Patch, v1 (obsolete) — Splinter Review
BMO patch.  I'll wait until people take a look at the screenshots before asking for review.
Attachment #8773762 - Flags: review?(glob) → review+
Attachment #8773763 - Flags: review?(glob) → review+
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.
Attachment #8773765 - Flags: review+
Attachment #8773762 - Flags: review?(smacleod)
Attachment #8773763 - Flags: review?(smacleod)
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.
Attachment #8774956 - Flags: review?(smacleod)
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".
Attached patch Patch, v2 (obsolete) — Splinter Review
Updated patch, with humanized integers and "white-space: nowrap" at glob's suggestion.
Attachment #8773765 - Attachment is obsolete: true
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.
Attachment #8774956 - Flags: review?(smacleod)
Attachment #8774957 - Flags: review?(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
Attachment #8774957 - Flags: review?(glob) → review-
Attached patch Patch, v3 (obsolete) — Splinter Review
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.
Attachment #8774957 - Attachment is obsolete: true
Attachment #8776759 - Flags: review?(glob)
Attachment #8773762 - Flags: review?(dwalsh)
Attachment #8773763 - Flags: review?(dwalsh)
Filed bug 1291065 for the corresponding Review Board change
Attached patch Patch, v4Splinter Review
.mozreview-diff-link is no longer used.
Attachment #8776759 - Attachment is obsolete: true
Attachment #8776759 - Flags: review?(glob)
Attachment #8776803 - Flags: review?(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';
Attachment #8776803 - Flags: review?(glob) → review+
To git@github.com:mozilla-bteam/bmo.git
   04eaf29..df005ea  master -> master
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Product: bugzilla.mozilla.org → bugzilla.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.