-
-
reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js (Diff revision 1) Every time I see this I'm confused, and it takes me a minute to realize that this is for a potential series of comments+replies for a single region (I always wonder why this isn't iterating over serializedComments). At a minimum, can we rename this to "addCommentBlock" (since it's only adding one), and document what "serializedComments" is?
Update DiffCommentBlock to be an AbstractCommentBlock in DiffReviewable.
Review Request #4267 — Created June 26, 2013 and submitted
Update DiffCommentBlock to be an AbstractCommentBlock in DiffReviewable.
DiffCommentBlock is now based on the AbstractCommentBlock{View}, which
allows us to eliminate the majority of the code and take advantage of
all the niceties that the Reviewable infrastructure gives us.
Some of the AbstractCommentBlockView code had to be made more flexible
with regard to positioning of tooltips and update bubbles, but otherwise
it's basically unmodified.
This also ends up fixing a long-standing bug where a comment in an
expanded region would not show up again if the chunk is collapsed and
then re-expanded.
Most of the code from diffviewer.js is now gone with this change.
Tested this pretty thoroughly with some diffs. Loaded in old comments (published and drafts). Created new ones. Tried different ranges. Tested making a comment in a collapsed area, then collapsed and re-expanded. Saw the comment. Repeated that test with a comment from a page load. Unit tests pass.
-
-
reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js (Diff revision 1) i is defined twice.
-
reviewboard/static/rb/js/utils/propertyUtils.js (Diff revision 1) Is it really worth checking if the property value is already the same? Especially for html that seems like it might be more expensive than just always calling the valued version.
