Closed Bug 1101611 Opened 7 years ago Closed 5 years ago

Review comments in diff view should be more visible

Categories

(MozReview Graveyard :: Review Board: User Interface, defect, P4)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ted, Assigned: glob)

References

Details

(Whiteboard: [to be fixed in core])

Attachments

(4 files)

Right now when you add a comment to the diff view and click save it appears only as a tiny number in the left margin, like here:
https://reviewboard.mozilla.org/r/499/diff/

At first I thought my comment had been eaten by gremlins, but the info bar at the top of the page was enough to assuage me there. It would be nice to have a more visible indicator that there's a comment there, however.

Compare Rietveld's presentation:
https://breakpad.appspot.com/5714003/diff/590004/src/google_breakpad/processor/microdump.h#new-line-84

Here, comments are displayed inline, but collapsed to a single line that can be expanded on click.
This should be fixed upstream.  Is there a bug?
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(smacleod)
Resolution: --- → WONTFIX
On second thought, I'll reopen.  We can leave this open and track any issues that are filed against upstream.

smacleod says that there is some agreement that the current UI isn't great but that there haven't been any good alternatives proposed yet.  In particular, they don't like the rietvald way because it breaks up the code, especially if there are a lot of comments on a particular line.  It also isn't obvious if a comment applies to several lines as it is with the current UI.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → NEW
I don't think there is any particular bug about this upstream, it's more just known by the core team and we have it on a students project list.
Flags: needinfo?(smacleod)
Priority: -- → P3
Whiteboard: [to be fixed in core]
Priority: P3 → P4
Duplicate of this bug: 1172766
Attached image mock up โ€”
as a stop gap measure until upstream come up with a solution, how about drawing a single line across the diff to draw attention to the comments?

implementation is a single line of js:
$('.commentflag').parents('tr').css('border-top', '2px dotted #444');
smacleod - thoughts?
Flags: needinfo?(smacleod)
Attached image orange_comment_bubble.jpg โ€”
Alternative quickfix proposal: change the background color of the comment bubble to something more visible (orange in this mockup)
(In reply to Mauro Doglio [:mdoglio] from comment #7)
> Alternative quickfix proposal: change the background color of the comment
> bubble to something more visible (orange in this mockup)

the problem i have with that is, especially on wide screens, a small box in the left margin is something you need to know to look for.  the brighter colour is better than what we currently have, but i think it needs something more to draw a new user's eye towards it.
Product: Developer Services → MozReview
(In reply to Byron Jones โ€น:globโ€บ from comment #5)
> Created attachment 8720136 [details]
> mock up
> 
> as a stop gap measure until upstream come up with a solution, how about
> drawing a single line across the diff to draw attention to the comments?
> 
> implementation is a single line of js:
> $('.commentflag').parents('tr').css('border-top', '2px dotted #444');

This seems like a fine temporary fix to me (although implementation wise couldn't we just do that with a css selector?)
Flags: needinfo?(smacleod)
(In reply to Steven MacLeod [:smacleod] from comment #9)
> This seems like a fine temporary fix to me (although implementation wise
> couldn't we just do that with a css selector?)

unfortunately not; you can't select an element based on its children, and we need to style the <tr> that contains the .commentflag element.
Assignee: nobody → glob
Review comments appear only as a tiny number in the left margin.  This makes
them easy to overlook, especially when viewing side-by-side diffs where your
focus is on the right column.  This adds a dotted line across the diff view
to draw attention to the comments.

Review commit: https://reviewboard.mozilla.org/r/36001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36001/
Attachment #8722382 - Flags: review?(mdoglio)
Component: General → Review Board: User Interface
Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36001/#review32655

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:47
(Diff revision 1)
> +.comment-block-container {

can you please attach a screenshot?

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:4
(Diff revision 1)
> +  // Adjust the styling of comment blocks to draw attention to their
> +  // existance.  Unfortunately the template is opaque to us, so we
> +  // have to resort to MutationObserver shenanigans.

Is this because we don't know when the diff will be rendered? If so, you can append a task to the `diff_files` funcQueue:
```js
$.funcQueue('diff_files').add(function(){
  // your js code
}
```
[Here](https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0d4ebd4464bd7889ea8c28670fc2c1e9f76c8aee/pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js#22) is a working example in mozreview.
Attachment #8722382 - Flags: review?(mdoglio)
https://reviewboard.mozilla.org/r/36001/#review32655

> can you please attach a screenshot?

it's identical to the mock-up on the bug - https://bug1101611.bmoattachments.org/attachment.cgi?id=8720136
https://reviewboard.mozilla.org/r/36001/#review32655

> Is this because we don't know when the diff will be rendered? If so, you can append a task to the `diff_files` funcQueue:
> ```js
> $.funcQueue('diff_files').add(function(){
>   // your js code
> }
> ```
> [Here](https://dxr.mozilla.org/hgcustom_version-control-tools/rev/0d4ebd4464bd7889ea8c28670fc2c1e9f76c8aee/pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js#22) is a working example in mozreview.

i've been exploring this.

'diff_files' is ok, but it's triggered after all the diffs have been received, so on reviews that touch a lot of files there's a noticeable delay in rendering, and the content is shifted down one pixel for each line we draw.  mutation events occur immediately.

another issue is i cannot find any event that is triggered when a comment is added to a review.

how do you feel about going with mutation events, and i'll pitch this change upstream where it can be implemented correctly?
https://reviewboard.mozilla.org/r/36001/#review32655

> i've been exploring this.
> 
> 'diff_files' is ok, but it's triggered after all the diffs have been received, so on reviews that touch a lot of files there's a noticeable delay in rendering, and the content is shifted down one pixel for each line we draw.  mutation events occur immediately.
> 
> another issue is i cannot find any event that is triggered when a comment is added to a review.
> 
> how do you feel about going with mutation events, and i'll pitch this change upstream where it can be implemented correctly?

Yeah sounds good to me
https://reviewboard.mozilla.org/r/36001/#review32655

> it's identical to the mock-up on the bug - https://bug1101611.bmoattachments.org/attachment.cgi?id=8720136

I wanted to add a comment to the screenshot :p
https://reviewboard.mozilla.org/r/36001/#review32655

> I wanted to add a comment to the screenshot :p

i spoke with mauro about this yesterday - he proposed using the same colours as the comments instead of black (ie. blue for comments, green for drafts).  it's a great improvement and i'll do that in the next revision.  when i push that revision up i'll attach an updated screenshot.

> Yeah sounds good to me

https://hellosplat.com/s/beanbag/tickets/4192/

if they don't like it, i'll propose triggering an event when a comment is added or removed (either during the initial render, or after the page is loaded) that we can listen for.
Attachment #8722382 - Flags: review?(mdoglio)
Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36001/diff/1-2/
Comment on attachment 8723424 [details]
MozReview Request: mozreview: fix indentation (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36547/#review33365
Attachment #8723424 - Flags: review?(mdoglio) → review+
Comment on attachment 8722382 [details]
MozReview Request: mozreview: draw attention to review comments (bug 1101611) r?mdoglio

https://reviewboard.mozilla.org/r/36001/#review33367
Attachment #8722382 - Flags: review?(mdoglio) → review+
https://hg.mozilla.org/hgcustom/version-control-tools/rev/2d5d156dcb24
Status: NEW → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
I had to back this out due to deployment failure:

TASK: [Generate static Review Board files] ************************************
failed: [reviewboardadm.private.scl3.mozilla.com] => {"changed": true, "cmd": ["/data/reviewboard/src/reviewboard.mozilla.org/venv/bin/rb-site", "manage", "/data/reviewboard/src/reviewboard.mozilla.org/reviewboard", "--", "collectstatic", "--noinput"], "delta": "0:00:09.498140", "end": "2016-03-01 22:30:45.967927", "rc": 1, "start": "2016-03-01 22:30:36.469787", "warnings": []}
stderr: Traceback (most recent call last):
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/bin/rb-site", line 11, in <module>
    sys.exit(main())
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 1922, in main
    command.run()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 1848, in run
    site.run_manage_command(args[0], args[1:])
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/cmdline/rbsite.py", line 676, in run_manage_command
    execute_from_command_line([__file__, cmd] + params)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/__init__.py", line 399, in execute_from_command_line
    utility.execute()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/__init__.py", line 392, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 242, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 285, in execute
    output = self.handle(*args, **options)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/core/management/base.py", line 415, in handle
    return self.handle_noargs(**options)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 173, in handle_noargs
    collected = self.collect()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/django/contrib/staticfiles/management/commands/collectstatic.py", line 119, in collect
    for original_path, processed_path, processed in processor:
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/storage.py", line 32, in post_process
    packager.pack_stylesheets(package)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 94, in pack_stylesheets
    variant=package.variant, **kwargs)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 103, in pack
    paths = self.compile(package.paths, force=True)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/packager.py", line 97, in compile
    return self.compiler.compile(paths, force=force)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 55, in compile
    return list(executor.map(_compile, paths))
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/_base.py", line 581, in result_iterator
    yield future.result()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/_base.py", line 405, in result
    return self.__get_result()
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/concurrent/futures/thread.py", line 55, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 40, in _compile
    outdated=outdated, force=force)
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/less.py", line 22, in compile_file
    return self.execute_command(command, cwd=dirname(infile))
  File "/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/pipeline/compilers/__init__.py", line 99, in execute_command
    raise CompilerError(stderr)
pipeline.exceptions.CompilerError: NameError: variable @STATIC_ROOT is undefined in /data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/mozreview-0.1.2a0-py2.6.egg/mozreview/static/mozreview/css/viewdiff.less on line 1, column 21:
1 @import (reference) "@{STATIC_ROOT}rb/css/defs.less";
2
stdout: Copying '/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/static/rb/css/syntax.css'
Copying '/data/reviewboard/src/reviewboard.mozilla.org/venv/lib/python2.6/site-packages/reviewboard/static/rb/css/pages/image-review-ui.css'
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> NameError: variable @STATIC_ROOT is undefined

urgh, it works in our dev/test env, but not on production.
i'll inline the colours.
https://hg.mozilla.org/hgcustom/version-control-tools/rev/e256ef207893
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.