Commenting functionality for text-based review UIs

Review Request #3613 — Created Dec. 2, 2012 and submitted

aam1r
Review Board
master
reviewboard
slchen
Implement functionality to allow text-based rendered review UIs to be commented on. This would allow users to comment on Markdown, XML, and RST rendered UIs (to name a few).

The functionality is done by:
- Creating a TextBasedReviewable. This inherits from FileAttachmentReviewable. All text-based UIs must now inherit from TextBasedReviewable for commenting functionality.
- Creating a TextBasedCommentBlock view and model. These comment block view is used in TextBasedReviewable as the comment block view.

In TextBasedReviewable, each rendered element is wrapped with <div class="text-review-ui-container" data-child-id="{id of child}"></div> (Note: this is not done recursively. If there is a paragraph with multiple nested hyperlinks, only the paragraph will be wrapped -- not the nested hyperlinks). The id that is assigned is unique auto-incremented ID. If there are n elements that will be wrapped, the IDs will range from [0..n-1].

A ghost comment icon is also displayed for the comments. This styling for this has been borrowed from diffviewer. 
This patch was applied on r/3434 (Markdown Review UI) and all testing was done on that review UI.

- Created single comment
- Created multiple comments on the same element
- Created multiple comments on different elements

(Did the above 3 with issues as well)

In all tests, everything saved and published successfully. Upon refreshing the rendered UI, the comments were displayed in the ghost comment icon and mouseover/click worked. 

Additionally, I made sure that nothing was broken in side-by-side diff after I made changes to diffviewer.less.

This was tested on Chrome on Mac.
Loading file attachments...

Description From Last Updated

We should have some additional padding for all elements on the left so these won't overlap.

chipx86chipx86

This looks like it'll interfere with the diff viewer.

chipx86chipx86

Can you include a comment block describing this?

chipx86chipx86

Should this subclass FileAttachmentCommentBlockModel instead of Abstract?

SL slchen

The function you provide is the same signature as _addCOmmentBlock. Just pass that directly.

chipx86chipx86

Only one var statement. The variables should be comma-separated, aligned, one per line.

chipx86chipx86

Same here.

chipx86chipx86

This should be a style, not hard-coded.

chipx86chipx86

Same about the style.

chipx86chipx86

We can probably axe this newline.

mike_conleymike_conley

So this is going to have a funky layout if one of the parent elements is an inline element... But ...

mike_conleymike_conley

This will actually be quite slow. You should just be doign a wrap() on the element we're handling.

chipx86chipx86
AA
AA
SL
  1. 
      
  2. Should this subclass FileAttachmentCommentBlockModel instead of Abstract?
    1. Hm, I think you mean FileAttachmentCommentBlockView. I originally intended to that but no such view exists so I inherited directly from AbstractCommentBlockView. I can create a "FileAttachmentCommentBlockView" but it won't have anything custom in it.
      
      There is, however, a FileAttachmentCommentBlock which is the model and TextCommentBlock is inheriting from that.
  3. 
      
chipx86
  1. 
      
  2. We should have some additional padding for all elements on the left so these won't overlap.
    1. Should I modify diffviewer.less to add the extra padding? If not, where should I do it so that it only applies to the rendered UI (and not diff viewer)?
    2. I'd figure out a class you can use for the container and use that as the base for the cells you want to add padding for. Something like "text-review-ui"
  3. reviewboard/static/rb/css/diffviewer.less (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This looks like it'll interfere with the diff viewer.
    1. Would I be better off doing this instead (for examples, lines 339-341):
      
      .selected .commentflag .commentflag-inner,
      .commentflag .commentflag-inner {
        background-color: @comment-flag-color;
      }
      
      or is this a better way of going about this?
    2. No, because that's the exact same thing.
      
      They should keep the .selected, but if you want something for your own use without setting a "selected" on the hovered row, you should have a version with a prefix using the above class I mentioned.
  4. Can you include a comment block describing this?
    1. Gotcha. I will add comment blocks today to other methods as well.
  5. The function you provide is the same signature as _addCOmmentBlock. Just pass that directly.
  6. Only one var statement. The variables should be comma-separated, aligned, one per line.
  7. This should be a style, not hard-coded.
    1. What less file should I create the style in? reviews. less?
    2. reviews.less.
  8. Same about the style.
  9. 
      
AA
AA
mike_conley
  1. Just two nits. I'm pretty happy with this.
  2. reviewboard/static/rb/css/diffviewer.less (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Um...weird that RB isn't noticing the indentation change here.
  3. We can probably axe this newline.
  4. reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    So this is going to have a funky layout if one of the parent elements is an inline element...
    
    But we'll solve that case later.
  5. 
      
AA
mike_conley
  1. I'm good with this. Thanks Aamir!
  2. 
      
mike_conley
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. This will actually be quite slow. You should just be doign a wrap() on the element we're handling.
    1. Or, even better, build a new element, but don't pack it. Append to that. Then replace the contents of this.$el with that element.
    2. Hm, I tried
      
      $(this).wrap("<div class='text-review-ui-container'></div>");
      
      but that didn't work. I believe .wrap() only works for elements that exist in the DOM.
    3. Yeah, do the second thing. Append to a new parent-less div. Then at the end, replace the contents (this.$el.empty().append($content_div))
    4. Christian,
      
      this.$el is undefined. Instead, I just tried doing this:
      
        var appended = $(this).appendTo("body");
        appended.wrap("<div class='text-review-ui-container'></div>");
      
      but that didn't work either. Could you emphasize a bit more on your example? I am not sure what you mean by append to a new parent-less div (I am already doing that in the existing code). 
    5. I don't know where you hit the undefined, but if it was inside the each(), your 'this' scoped changed, which is where you'd use 'self'.
      
      In applyCommentWrapper, you should do:
      
      var $wrapper = $('<div/>');
      
      this._$rendered.each(function() {
          var $child = blah
              .blah
              .appendTo($wrapper);
      });
      
      this.$el
          .empty()
          .append($wrapper);
      
      
      While we're at it, your attr() call setting the class should be addClass() instead.
    6. After that, there's no reason to declare self anymore.
      
      And actually, rather than empty().append(..), you should be able to do:
      
      
          this.$el.children().replaceWith($wrapper);
      
      
      Then below, where you're doing this.$el.find('data-blah'), you should do .children('data-blah'), as it won't have to search all children, just the top-level elements.
  3. 
      
AA
david
  1. Ship It!
  2. 
      
AA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (ce0c087). Thanks!
Loading...