Move the reply draft banner out into its own view.
Review Request #4201 — Created June 4, 2013 and submitted
Move the reply draft banner out into its own view. The reply draft banner has been moved out of reviews.js and into ReviewReplyDraftBannerView. The new code is more "dumb" in that it doesn't deal with anything beyond rendering and handling the publish/discard. It's up to the caller to determine what happens after (such as page reloading). The floating support moved into FloatingBannerView, which ReviewReplyDraftBannerView inherits from. This has been made a bit more generic, so that it can be use for other banners later on.
Tested all the floating semantics. We're bug-for-bug compatible with what we had before all the JavaScript refactoring (tested using RBCommons). Tested that the banner appeared after a new comment. Tested both publishing and discarding, both after an initial new comment (when there wasn't previously a banner), and after a page load with an existing draft comment.
| Description | From | Last Updated |
|---|---|---|
|
There's a weird leading space here. |
 david
|
|
|
Instead of defining $buttons as a variable, how about just using this.$('input') inside the callback? |
david
|
|
|
At the moment, FloatingBannerView doesn't do anything to this.$el, but I'd rather not rely on that behavior. How about calling … |
david
|
|
|
Now that we've updated backbone, I'm tempted to say we should do this: this.listenTo(reviewReply, 'destroy', function() { ... }); ... … |
david
|
-
-
-
Instead of defining $buttons as a variable, how about just using this.$('input') inside the callback? -
At the moment, FloatingBannerView doesn't do anything to this.$el, but I'd rather not rely on that behavior. How about calling the prototype's render at the beginning of this method instead of the end?
- Change Summary:
-
* Added a missing period in the header text. * Removed $buttons. * Moved the parent render call to the top of the function. It was at the bottom because originally it needed a rendered element before being invoked.
- Diff:
-
Revision 2 (+265 -157)
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/settings.py Ignored Files: reviewboard/static/rb/js/models/reviewReplyModel.js reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js reviewboard/static/rb/js/views/floatingBannerView.js reviewboard/static/rb/js/views/reviewReplyEditorView.js reviewboard/static/rb/js/models/reviewReplyEditorModel.js reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js reviewboard/static/rb/js/reviews.js reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js
-
-
Now that we've updated backbone, I'm tempted to say we should do this: this.listenTo(reviewReply, 'destroy', function() { ... }); ... Using listenTo means that when this object is destroyed it will unhook all it's listeners on other objects, and you don't need to explicitly pass this as the context.
- Change Summary:
-
Switched to listenTo.
- Diff:
-
Revision 3 (+265 -157)
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/settings.py Ignored Files: reviewboard/static/rb/js/models/reviewReplyModel.js reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js reviewboard/static/rb/js/views/floatingBannerView.js reviewboard/static/rb/js/views/reviewReplyEditorView.js reviewboard/static/rb/js/models/reviewReplyEditorModel.js reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js reviewboard/static/rb/js/reviews.js reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/settings.py Ignored Files: reviewboard/static/rb/js/models/reviewReplyModel.js reviewboard/static/rb/js/views/tests/reviewBoxViewTests.js reviewboard/static/rb/js/views/floatingBannerView.js reviewboard/static/rb/js/views/reviewReplyEditorView.js reviewboard/static/rb/js/models/reviewReplyEditorModel.js reviewboard/static/rb/js/views/reviewReplyDraftBannerView.js reviewboard/static/rb/js/reviews.js reviewboard/static/rb/js/views/reviewBoxView.js reviewboard/static/rb/js/views/tests/reviewReplyDraftBannerViewTests.js