Summary

Display overlapping comment bubbles side by side

Review Request #6970 — Created Feb. 22, 2015 and submitted April 28, 2016, 2:48 a.m.

Information

Owner

wudi

Repository

Review Board

Branch

master

Bugs

Depends On

Commit

c2c7538...

Reviewers

Groups

reviewboard, students

People

Description

This is the second part in improving the diff comment bubbles.

Multiple comment bubbles on the same range will end up overlapping, making it hard to click or inspect on the comments below. This change allows the overlapping comment bubbles to spread out side-by-side when the mouse hovers above them.

Testing Done

Ran js tests.

Manually testing:
- Added multiple overlapping comment bubbles on the same line to check if the bubbles spread out and collapse correctly.
- Checked tooltips are all displayed on the right side of the range of comment bubbles.
- Checked that clicking the comment bubbles brings up the comment editor.

Files

Issues

Description	From	Last Updated
This shoud go at the top of the file.	chipx86	Feb. 23, 2015, 7:02 a.m.
Can you make these constants on the class, and document them? I'm not sure where the numbers are coming from. …	chipx86	Feb. 23, 2015, 7:02 a.m.
Since this is a jQuery object, it should be $parent. It is just a convention we use.	brennie	Feb. 26, 2015, 11:08 p.m.
We're calling $el.parent() more than once. Best to store the result in a variable and re-use it.	chipx86	Feb. 23, 2015, 7:02 a.m.
All declared variables must be at the top of the function.	chipx86	Feb. 23, 2015, 7:02 a.m.
Comparison should be !==	chipx86	Feb. 26, 2015, 11:07 p.m.
Unnecessary parens (like around the multiplication) can be removed.	chipx86	Feb. 23, 2015, 7:02 a.m.
Alignment issue with the variable values. Also, this is a bit ahrd to read. How about: var leftMargin = ..., …	chipx86	Feb. 23, 2015, 7:02 a.m.
You should chain these, so you don't have to wrap bubble more than once.	chipx86	Feb. 23, 2015, 7:02 a.m.
Same comment about the initial values. Since we're defining these in more than once place, that's all the more reason …	chipx86	Feb. 23, 2015, 7:02 a.m.
Only one var statemnet per function. Also, like above, we only want to call $el.parent() once.	chipx86	Feb. 23, 2015, 7:02 a.m.
Likewise, this too should be $parent.	brennie	Feb. 26, 2015, 11:08 p.m.
That key is pretty generic. Let's use a more specific key. Also, the key should be defined as a constant …	chipx86	Feb. 23, 2015, 7:02 a.m.
Space after >.	chipx86	March 16, 2015, 9:13 a.m.
Space around >. Same with others.	chipx86	March 16, 2015, 9:13 a.m.
Are you sure this always does the right thing? Have you tested different zoom levels?	chipx86	March 16, 2015, 9:22 a.m.
Comma should go before the comment. Space before the comment, and space between the text and the commnt markers. "Pixels" …	chipx86	March 16, 2015, 9:16 a.m.
The two lines of values for width must align.	chipx86	March 16, 2015, 9:16 a.m.
Would be cleaner as (commentBubbles.length - index - 1).	chipx86	March 16, 2015, 9:16 a.m.
You can simplify this by doing: .css('width', width + 'px')	chipx86	March 16, 2015, 9:16 a.m.
This repeats the same logic as above. This will work better as a utility function.	chipx86	March 16, 2015, 9:17 a.m.
Blank line between these.	chipx86	March 16, 2015, 9:17 a.m.
You could probably just pass the result of the setTimeout call inline in here, making this an the above one …	chipx86	March 16, 2015, 9:17 a.m.
Why this change?	brennie	April 14, 2015, 3:54 a.m.
Can we just use this.$el throught here instead of introducing $el ?	brennie	April 14, 2015, 3:49 a.m.
Same here.	brennie	April 14, 2015, 3:49 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Groups:: students

Change Summary:


Change css selected row color to only affect it's immediate children.
Change for loop to jquery's each function.

Commit:

8d4741a174cbec9b71952c179f3f0384eb4cfd11

198cfab1e1fdcac0bae0b1f6ff1275a4dd6e7d17

Diff:

Revision 2 (+88 -5)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
The issue has been resolved. Show all issues
```
This shoud go at the top of the file.
```

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

The issue has been resolved. Show all issues

Can you make these constants on the class, and document them? I'm not sure where the numbers are coming from.

If these are things that can be calculated (I'm guessing the initialWidth/initialLeft are), then we should calculate them instead.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
We're calling $el.parent() more than once. Best to store the result in a variable and re-use it.
```
reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
All declared variables must be at the top of the function.
```

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

The issue has been resolved. Show all issues

Comparison should be !==

wudi

Feb. 23, 2015, 5:37 a.m.

timeoutId is defined in the mouseleave event, and will be set to undefined after 500ms. Thus a defined timeoutId will mean that the bubbles are not collapsed, the bubbles should spread out only when it is undefined.

chipx86

Feb. 26, 2015, 5:42 p.m.

I mean, in JavaScript, you want to use the !== operator instead of the != operator. The first is for strict checking, and the latter will cast first (such that 1 != "1" will fail).

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Unnecessary parens (like around the multiplication) can be removed.
```

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

The issue has been resolved. Show all issues

Alignment issue with the variable values.
Also, this is a bit ahrd to read. How about:

var leftMargin = ...,
     width = initialWidth +
             spreadDistance * (commentBubbles.length - index + 1);

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

The issue has been resolved. Show all issues

You should chain these, so you don't have to wrap bubble more than once.

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

The issue has been resolved. Show all issues

Same comment about the initial values.

Since we're defining these in more than once place, that's all the more reason to have a single place where we define or calculate them.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Only one var statemnet per function.
Also, like above, we only want to call $el.parent() once.
```

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

The issue has been resolved. Show all issues

That key is pretty generic. Let's use a more specific key.

Also, the key should be defined as a constant somewhere, and reused.

Commit:

198cfab1e1fdcac0bae0b1f6ff1275a4dd6e7d17

fac3d5a00b2cb996d9a5717cb17c7b64b5580dd9

Diff:

Revision 3 (+102 -8)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

What happens if you mouse in/out very quickly over the bubbles? If it starts acting like an accordion, you may want to look into using HoverIntents (which we already have packaged with RB).

wudi

Feb. 26, 2015, 10:47 p.m.

By acting like an accordion do you mean that the bubbles require clicking to expand out? As of now it will expand out on hover, even if the mouse move in/out very quickly over the bubbles, should this be the intended behavior? I think it's reasonable since it's very easy to miss out the bubbles buried below if they do not instantly expand out.

brennie

Feb. 27, 2015, 8:43 a.m.

So I manually tested your change and if you mouse in and out over the same comment bubble quickly, it will expand, pause, contract, and then repeat the process. You should look into using HoverIntents, which we already package with RB and is used in the diffFragmentQueueView. It can prevent the behaviour I mentioned.

wudi

March 3, 2015, 6:51 a.m.

I attempted to use HoverIntents but found that it was not very suitable for this case.

For every mouseleave event, I perform a check to see if the mouse is hovering over the sibling comment bubbles before collapsing all of them. HoverIntents will prevent the animation chaining effect but cannot be used to detect whether the mouse is hovering over the sibling comment bubbles if no mousemovent is made.

So when the cursor moves over the overlapping bubbles, they will expand out, the mouseleave event will trigger for the topmost bubble, but as the cursor stays put, no mouseover event is triggered for the bubbles below, causing all of them to collapse even when the cursor is hovering over them.

In the end I used another method to circumvent the animation chaining by stopping all animations before starting a new one, moving the mouse in/out very quickly will no longer cause the animations to build up.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
Since this is a jQuery object, it should be $parent. It is just a convention we use.
```
reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revisions 2 - 3)
The issue has been resolved. Show all issues
```
Likewise, this too should be $parent.
```

Commit:

fac3d5a00b2cb996d9a5717cb17c7b64b5580dd9

4acc9ec5ee81b107cdd76a98072ac8727b42ab1b

Diff:

Revision 4 (+102 -8)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Change Summary:

Prevent chaining of animation effects when the mouse move in/out of the comment bubble very quickly

Commit:

4acc9ec5ee81b107cdd76a98072ac8727b42ab1b

2ee9a5425527f57c162fdcadad39f7ede94eb3b1

Diff:

Revision 5 (+104 -8)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Space after >.
```
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Space around >.
Same with others.
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 5)

The issue has been dropped. Show all issues

Are you sure this always does the right thing? Have you tested different zoom levels?

wudi

March 16, 2015, 9:22 a.m.

@commentflag-width is set to 16em, but since the font-size is set to 90% for .commentflag-inner, the width (in pixels) is also decreased. By dividing it by 90%, the actual calcuated value will be around 1.78em. I have verified that the width is correct for different zoom levels.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)

The issue has been resolved. Show all issues

Comma should go before the comment.

Space before the comment, and space between the text and the commnt markers.

"Pixels" should be capitalized.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
The two lines of values for width must align.
```
reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Would be cleaner as (commentBubbles.length - index - 1).
```

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)

The issue has been resolved. Show all issues

You can simplify this by doing:

.css('width', width + 'px')

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)

The issue has been resolved. Show all issues

This repeats the same logic as above. This will work better as a utility function.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)

The issue has been resolved. Show all issues

You could probably just pass the result of the setTimeout call inline in here, making this an the above one statement.

Commit:

2ee9a5425527f57c162fdcadad39f7ede94eb3b1

cb62356f0457eed6163f74fec5bc99177825585f

Diff:

Revision 6 (+104 -9)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/search.less
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractCommentBlockView.js
    reviewboard/static/rb/css/pages/search.less
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Ship it!

Looks good to me, just a few nitpicks and a question!

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 6)

The issue has been resolved. Show all issues

Why this change?

wudi

April 14, 2015, 3:54 a.m.

Was testing out with lots of overlapping comment bubbles, hovering over the rightmost bubble will cause the tooltip to display on its left and behind the rest of the bubbles.

Since lrbt is used for image comments, I've changed this back. To make the tooltips in text based views display on the right, I overrode the tooltipSides property to rlbt in textBasedCommentBlockView.

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 6)

The issue has been resolved. Show all issues

Can we just use this.$el throught here instead of introducing $el ?

reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Same here.
```

Change Summary:

Change tooltipSides property in abstractCommentBlockView back to prioritize left over right since that is the preference for image commenting.
For textBasedCommentBlockView, we would want the tooltips to always display on the right side of the comment block, especially when the comment bubbles are spread out to the right.

Commit:

cb62356f0457eed6163f74fec5bc99177825585f

c2c7538038a9f3b3030bf046fadb45b8605cc2e1

Diff:

Revision 7 (+102 -7)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/views/textBasedCommentBlockView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/textBasedCommentBlockView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Status:: Completed
Change Summary:: Pushed to students/wudi/overlapping-comment-bubbles (aa9dca2)

Status:: Re-opened

Status:: Completed
Change Summary:: Pushed to students/wudi/overlapping-comment-bubbles (33a4f94)