Summary

Rewrite our screenshot review UI and introduce generic review infrastructure.

Review Request #3342 — Created Sept. 23, 2012 and submitted Oct. 19, 2012, 12:14 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Rewrite our screenshot review UI and introduce generic review infrastructure.

The screenshot review support has been rewritten to have a nice
model/view separation and to abstract much of the review process into
generic models and views.

There's now a few new classes: AbstractCommentBlock, AbstractCommentBlockView,
AbstractReviewable, and AbstractReviewableView. These handle the basics
of creating review UIs and comment blocks.

The screenshot subclasses, as a result, are fairly minimal for the most
part.

From here on out, it should be easier to create review UIs for custom
mimetypes. This will get even easier as more of our review
infrastructure is moved to new Backbone models and views.

Testing Done

Tested all aspects of screenshot commenting. Loading, creating, deleting.
Drafts, non-drafts.

Issues

Description	From	Last Updated
Can you merge these? return $('') .appendTo(list) .text(comment.text.truncate());	david	Oct. 5, 2012, 6:16 p.m.
===?	david	Oct. 5, 2012, 6:17 p.m.

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you merge these?

return $('<li>')
       .appendTo(list)
       .text(comment.text.truncate());
```

reviewboard/static/rb/js/views/abstractReviewableView.js (Diff revision 1)

Should this be the same this?

this this this?

chipx86 Oct. 5, 2012, 6:17 p.m.

It is, but it's implicit due to listening to this's event. I'm going to add an explicit context (this) as a parameter to once() so we can guarantee 'this'.

reviewboard/static/rb/js/views/screenshotReviewableView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
===?
```

Change Summary:

* Synced to master.
* Merged some statements by using jQuery chaining.
* Explicitly passed a context to a call to once() for 'this'.
* Changed a comparison to use ===.

Diff:

Revision 2 (+618 -402)

Show changes

	reviewboard/settings.py
	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/js/screenshots.js
	reviewboard/static/rb/js/models/abstractReviewableModel.js
	reviewboard/static/rb/js/models/screenshotReviewableModel.js
	reviewboard/static/rb/js/utils/backboneUtils.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/abstractReviewableView.js
	3 more

Ship it!

```
Ship It!
```

Status:: Completed