chipx86 got a fish trophy!

Summary

Rewrite the review dialog.

Review Request #4224 — Created June 8, 2013 and submitted June 9, 2013, 11:46 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Rewrite the review dialog.

The review dialog has had a full rewrite. It's no longer just a
container for a server-side rendered form, which was done in a very
hacky way and could be slow to load for large changes. Now it displays
instantly and fetches the comments itself, rendering them one at a time.

This looks the same as the old form, but with one improvement:
File attachment thumbnails are now rendered as well. They appear just
like screenshot thumbnails do. Previously, we'd only render the
caption or filename.

Unit tests were added for this.

Testing Done

Unit tests pass.

Tested all three types of comments.

Tested modifying every type of field on the reviews and comments and making sure they persisted on save.

Tested publishing.

Tested discarding the draft.

Tested creating a brand new review from scratch with the Review button, and publishing that.

Issues

Description	From	Last Updated
For now (until we use require.js), can we wrap everything in an anonymous function to avoid polluting the global namespace?	david	June 9, 2013, 3:46 p.m.
If this gets called before render() it'll raise an exception. Can you check whether these are null?	david	June 9, 2013, 1:42 p.m.
Can we use this.listenTo?	david	June 9, 2013, 3:50 p.m.
this.listenTo?	david	June 9, 2013, 3:51 p.m.
this.listenTo?	david	June 9, 2013, 3:51 p.m.
alert?	david	June 9, 2013, 3:53 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/urls.py
  Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/review_draft_inline_form.html

Fix it!

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

The issue has been resolved. Show all issues

For now (until we use require.js), can we wrap everything in an anonymous function to avoid polluting the global namespace?

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

An issue was opened. Show all issues

If this gets called before render() it'll raise an exception. Can you check whether these are null?

chipx86 June 9, 2013, 4:25 p.m.

I could, but they won't be called before render. They're internal only and only called during save(). I'm not sure it's worth the extra code in this case unless we end up exposing these classes later.

reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Can we use this.listenTo?
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
this.listenTo?
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
this.listenTo?
```
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
alert?
```

Change Summary:

* Added a closure around the file.
* Switched to listenTo().
* Removed an alert for error reporting. We'll just fall back on the RB.apiCall error handling.

Diff:

Revision 2 (+1107 -436)

Show changes

	reviewboard/settings.py
	reviewboard/reviews/urls.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/js/reviews.js
	reviewboard/static/rb/js/views/draftReviewBannerView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/settings.py
    reviewboard/reviews/urls.py
  Ignored Files:
    reviewboard/static/rb/js/views/draftReviewBannerView.js
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/js/views/tests/draftReviewBannerViewTests.js
    reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/reviews.js
    reviewboard/templates/reviews/review_draft_inline_form.html

Ship it!

```
Ship It!
```

Status:: Completed