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.

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)?

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"

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?

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.

Gotcha. I will add comment blocks today to other methods as well.

What less file should I create the style in? reviews. less?

reviews.less.

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.

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.

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

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

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.

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.