Commenting functionality for text-based review UIs
Review Request #3613 — Created Dec. 2, 2012 and submitted
Information | |
---|---|
aam1r | |
Review Board | |
master | |
Reviewers | |
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.
Description | From | Last Updated |
---|---|---|
We should have some additional padding for all elements on the left so these won't overlap. |
|
|
This looks like it'll interfere with the diff viewer. |
|
|
Can you include a comment block describing this? |
|
|
Should this subclass FileAttachmentCommentBlockModel instead of Abstract? |
SL slchen | |
The function you provide is the same signature as _addCOmmentBlock. Just pass that directly. |
|
|
Only one var statement. The variables should be comma-separated, aligned, one per line. |
|
|
Same here. |
|
|
This should be a style, not hard-coded. |
|
|
Same about the style. |
|
|
We can probably axe this newline. |
|
|
So this is going to have a funky layout if one of the parent elements is an inline element... But … |
|
|
This will actually be quite slow. You should just be doign a wrap() on the element we're handling. |
|
-
-
reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 1) Should this subclass FileAttachmentCommentBlockModel instead of Abstract?
-
-
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 1) This looks like it'll interfere with the diff viewer.
-
reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 1) Can you include a comment block describing this?
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 1) The function you provide is the same signature as _addCOmmentBlock. Just pass that directly.
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 1) Only one var statement. The variables should be comma-separated, aligned, one per line.
-
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 1) This should be a style, not hard-coded.
-
Change Summary:
- Added detailed comments and made some stylistic changes to JS - Moved comment flag color variables from diffviewer.less to defs.less. This way they can be used in multiple files without redeclaring them - Reverted the change in diffviewer.less where I removed the ".selected" selector from certain lines - Renamed "rendered-comment" to "text-review-ui-container" - Added styling for "text-review-ui-container" in reviews.less
Change Summary:
Updated description and "Testing Done" to include that I made sure that nothing was broken in side-by-side after I made changes to diffviewer.less
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
Just two nits. I'm pretty happy with this.
-
reviewboard/static/rb/css/diffviewer.less (Diff revision 2) Um...weird that RB isn't noticing the indentation change here.
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2) We can probably axe this newline.
-
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.
Change Summary:
Addressed issues
Diff: |
Revision 3 (+257 -54)
|
---|
-
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 3) This will actually be quite slow. You should just be doign a wrap() on the element we're handling.
Change Summary:
Instead of appending the outerHTML to a string, we now use a jQuery object and append each wrapped comment to it. Additionally, we use .children() instead of .find() to make child look ups faster.
Diff: |
Revision 4 (+256 -54)
|
---|