Bug 45870 – [reviewtool] should always show overall comments text box

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ojan Vafai]

I'd prefer for the overall comments text box to always be visible and aligned to the bottom. I find I often want to accumulate overall comments as I'm going through a review. Also, it's annoying having the overall comments box in the middle of the page for small reviews. I would just inine the text box to the right of the EWS iframe

[Adam Barth]

Interesting idea.  We'll have to make sure its not taking too much space on the screen.

[Ojan Vafai]

Created attachment 75945 [details]
Patch

[Ojan Vafai]

Created attachment 75946 [details]
before screenshot

[Ojan Vafai]

Created attachment 75947 [details]
after screenshot

[Adam Barth]

Comment on attachment 75945 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75945&action=review

> BugsSite/ChangeLog:11
> +        (.):
> +        ():

I usually remove these lines from the ChangeLog because they're kine of meaningless.

> BugsSite/PrettyPatch/PrettyPatch.rb:210
> +  display: -webkit-box;
> +  display: -moz-box;

Woah, you're blowing my mind.

> BugsSite/code-review.js:250
> -      $('#toolbar .actions').append($('<iframe class="statusBubble" src="https://webkit-commit-queue.appspot.com/status-bubble/' + attachment_id + '" scrolling="no"></iframe>'));
> +      $('#toolbar .actions').append($('<br><iframe class="statusBubble" src="https://webkit-commit-queue.appspot.com/status-bubble/' + attachment_id + '" scrolling="no"></iframe>'));

So, actually, I would put the bubbles above the preview / publish buttons.  That's because I expect people to look at the bubbles before clicking the buttons, so they should be visually linear instead of requiring the user's eyes to move down and then up again.

> BugsSite/code-review.js:275
> +        '<div class="links"><span class="bugLink"></span></div>' +

If I had my way, the bug link would be aligned to the bottom right corner instead of the top right.

[Adam Barth]

I'm slightly hesitating to r+ this patch because the new design makes the toolbar larger.  I have a dream that one day we're merge Review Patch and Formatted Diff into one screen, and having a heavy toolbar works against that goal.

I definitely think you're solving and important problem.  Have you considered a design where the overall comments box appears above the toolbar when you mouse over the toolbar (and disappears when you mouse out)?  That way you can have access to it wherever you are in the review but it doesn't contribute weight to the toolbar when you're not using it.

[Ojan Vafai]

Created attachment 76116 [details]
Patch

[Ojan Vafai]

Created attachment 76117 [details]
minimized case screenshot

[Ojan Vafai]

Created attachment 76118 [details]
expanded case screenshot

You see this after clicking in the overall comments textarea.

[Adam Barth]

I appreciate your working on the review tool.  Like many user interface judgements, it comes down to personal taste and aesthetics.  Let me try to work up some mockups of another approach so we can see what we think.

[Adam Barth]

Created attachment 76258 [details]
alternative expanded view

[Ojan Vafai]

I don't like the toolbar being large like that for people who use the overall comments box. There's not much advantage of it taking up the whole width of the window. You rarely type lines that long.

Is your issue with the current version the amount of whitespace in the middle? That's a byproduct of the status bubble iframe being so large. I was thinking we could use postMessage to have the iframe tell the outer page the size of it's contents so it could size correctly. Or, we could implement seamless. :)

[Adam Barth]

(In reply to comment #12)
> I don't like the toolbar being large like that for people who use the overall comments box. There's not much advantage of it taking up the whole width of the window. You rarely type lines that long.

That's true.  We could shrink it down horizontally to something more reasonable.

> Is your issue with the current version the amount of whitespace in the middle?

My issue is more the visual flow.  If you trace the path you expect users to follow with their eyes, you want the path to be as smooth as possible (and ideally top-to-bottom or left-to-right).  With the placement you've got, the trace has to go back and forth a bunch of times.

That points to putting the comment box above the publish button so the visual flow is top-to-bottom, which is why I tried to show it appearing above the button.  Another option is to move the buttons to the right so the flow is left-to-right with comment box => bubbles => publish.

[Ojan Vafai]

Created attachment 76614 [details]
Patch

[Ojan Vafai]

Created attachment 76615 [details]
minimized case screenshot

[Ojan Vafai]

Created attachment 76616 [details]
expanded case screenshot

[Ojan Vafai]

How's this?

[Adam Barth]

I like the approach.  Is there a reason the buttons move when you expand the text field?  That seems like it might be slightly disorienting.

[Ojan Vafai]

(In reply to comment #18)
> I like the approach.  Is there a reason the buttons move when you expand the text field?  That seems like it might be slightly disorienting.

The statusBubble goes above the buttons just to save space. I tried keeping the buttons aligned right, but it looks really odd with the whitespace to the right of the statusBubble. The whitespace comes from the fact that the statusBubble is an iframe and not sized to it's content, which, as mentioned earlier, we could fix with postMessage or by implementing seamless.

We could not put the statusBubble above the buttons, but that gives less horizontal space for the comments section on small screens.

Alternately, I could just make the statusBubble smaller to actually fit it's current contents. Then in the future if we add more items we'll need to make it larger/smaller.

[Adam Barth]

> Alternately, I could just make the statusBubble smaller to actually fit it's current contents. Then in the future if we add more items we'll need to make it larger/smaller.

That seems valuable.  We have the same problem on the show_bug page where the status bubbles are stretching out the layout.

[Adam Barth]

Comment on attachment 76614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76614&action=review

I'm not sure happy with the new UI, but I think that's just because I fear losing control of the reviewtool and not because of any real substantive issue.  A couple nits below.  If you'd like to implement the auto-sizing of the status bubble, that would be great.

> BugsSite/code-review.js:266
> +  function openOverallComments(e) {
> +    document.querySelector('.overallComments textarea').className = 'open';
> +    document.querySelector('#statusBubbleContainer').className = 'wrap';
> +  }

Should we use jQuery here?

> BugsSite/code-review.js:287
> +    document.querySelector('.overallComments textarea').addEventListener('click', openOverallComments);

We really should use jQuery here.

[Ojan Vafai]

Committed r74130: <http://trac.webkit.org/changeset/74130>