Display overlapping comment bubbles side by side

Review Request #6970 - Created Feb. 22, 2015 and submitted

Wu Di
Review Board
master
c2c7538...
reviewboard, students
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.
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.
Loading file attachments...

  • 0
  • 0
  • 25
  • 1
  • 26
Description From Last Updated
Review Bot
  1. 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
    
    
  2. 
      
Wu Di
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Christian Hammond
  1. 
      
  2. This shoud go at the top of the file.

  3. 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.

  4. We're calling $el.parent() more than once. Best to store the result in a variable and re-use it.

  5. All declared variables must be at the top of the function.

  6. Comparison should be !==

    1. 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.

    2. 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).

  7. Unnecessary parens (like around the multiplication) can be removed.

  8. 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);
    
  9. You should chain these, so you don't have to wrap bubble more than once.

  10. 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.

  11. Only one var statemnet per function.

    Also, like above, we only want to call $el.parent() once.

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

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

  13. 
      
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Barret Rennie
  1. 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).

    1. 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.

    2. 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.

    3. 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.

  2. Since this is a jQuery object, it should be $parent. It is just a convention we use.

  3. Likewise, this too should be $parent.

  4. 
      
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Christian Hammond
  1. 
      
  2. Space after >.

  3. Space around >.

    Same with others.

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

    1. @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.

  5. Comma should go before the comment.

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

    "Pixels" should be capitalized.

  6. The two lines of values for width must align.

  7. Would be cleaner as (commentBubbles.length - index - 1).

  8. You can simplify this by doing:

    .css('width', width + 'px')
    
  9. reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)
     
     
     
     
     
     
     
     
     

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

  10. Blank line between these.

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

  12. 
      
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Barret Rennie
  1. Looks good to me, just a few nitpicks and a question!

    1. 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.

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

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

  3. 
      
Wu Di
Review Bot
  1. 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
    
    
  2. 
      
Wu Di
Wu Di
Wu Di
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to students/wudi/overlapping-comment-bubbles (33a4f94)
Loading...