Refactor Review Request banners to be driven entirely from JavaScript.
Review Request #7338 — Created May 26, 2015 and submitted
The Review Request banners (close banners and draft banners) were a weird mix
of backend- and frontend-driven code. When rendering the review request page,
we'd render the banner from a Django template and then "import" it from the
javascript view. We'd also have some actions (such as changing a field or
marking the review request as closed) that would create a new banner entirely
from javascript. These two code paths used slightly different markup, and made
it annoying to think about and to test.This change updates things so that all of our banners are created entirely in
javascript. This involved quite a few changes:
- The
showBannerimplementation would unconditionally end up showing a draft
banner if the review request wasn't closed, because it had previously only
been called when doing something that would create a draft. We had an unused
hasDraftfield on theReviewRequestEditor, which I've repurposed for use
here in determining whether we should show a banner or not. - Updated the js-tests to account for the fact that we now need a draft present
in order for the draft banner to be created. - Because the initial text for the close description or draft change
description were rendered from django, we didn't have the rendered text
available in javascript. I've plumbed this through theReviewRequestEditor. - I ported over the "Send E-Mail" check-box and "This draft adds a new diff"
info box into the javascript-driven draft banner. - Tweaked the spacing a little bit in the draft banner with the "new diff"
link.
- Verified that opening up existing review requests with drafts, or closed
review requests (both submitted and discarded) would create the correct
banner, and that the change description/close description text was correctly
presented in both markdown and plain mode. - Verified that banners were created when making changes to fields or closing
the review request. - Ran unit tests.
- Ran js-tests.
| Description | From | Last Updated |
|---|---|---|
|
Blank line between these. |
 chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Multi-line comments should use /* .. * .. */. Might not be worth having this comment, though. No harm in … |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
This should be a template. |
chipx86
|
|
|
Since we're now going to be in this function without a banner sometimes, can we move all this into the … |
chipx86
|
|
|
How about we just return on else, and keep the assert? What that gives us is a check that we … |
chipx86
|
|
|
Can we use double quote for the view name, to fix highlighting? Also, I think we want to escape the … |
chipx86
|
|
|
Can we either keep this one line or indent 4 spaces? We're not wrapping on the other lines, so I … |
chipx86
|
|
|
If this is HTML and not text, we should call it descriptionFieldHTML, so that it's clear we don't want to … |
chipx86
|
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Multi-line comments should use
/* .. * .. */.Might not be worth having this comment, though. No harm in calling off the prototype.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) This should be a template.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) Since we're now going to be in this function without a banner sometimes, can we move all this into the block of code down below where we set up the BannerClass, so we don't have to do it otherwise?
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1) How about we just return on
else, and keep the assert?What that gives us is a check that we didn't assign
BannerClassto something undefined. (That would be more useful if we were to pull these out into their own files I guess.) -
reviewboard/templates/reviews/reviewable_page_data.js (Diff revision 1) Can we use double quote for the view name, to fix highlighting?
Also, I think we want to escape the URL. We can do that by doing
{% url ... as ... %}outside of the script, then pass this in with|escapejs. -
reviewboard/templates/reviews/reviewable_page_data.js (Diff revision 1) Can we either keep this one line or indent 4 spaces? We're not wrapping on the other lines, so I don't know that we need to here.
-
reviewboard/templates/reviews/reviewable_page_data.js (Diff revision 1) Totally unrelated to this change, but we really should make a
jsbooltemplate tag.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+150 -109) |
-
Tool: Pyflakes Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2) If this is HTML and not text, we should call it
descriptionFieldHTML, so that it's clear we don't want to escape this.
Tool: PEP8 Style Checker Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: Pyflakes Ignored Files: reviewboard/templates/reviews/review_detail.html reviewboard/templates/reviews/ui/base.html reviewboard/templates/reviews/reviewable_page_data.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/templates/diffviewer/view_diff.html reviewboard/static/rb/js/resources/models/draftReviewRequestModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/review_header.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js