Make the Review Dialog use inlineEditors.
Review Request #6239 — Created Aug. 17, 2014 and submitted
Traditionally, the Review Dialog (created either via the "Review" action or
"Edit Review" in the draft review header) had a text box for the body top and
body bottom headers, and a text box for each comment.This made a lot more sense when the comments were plain text, and the contents
of the text inputs was the same as what the user would see. One thing that
always bothered me about the addition of markdown formatting for comments was
that it was impossible to truly know what you'd get after a review was
published, since we never showed the comments in rendered form. It also meant
that we were potentially creating a huge number of CodeMirror instances, which
made the Review Dialog pretty slow.This change replaces all the text boxes with rendered text. This makes it more
or less resemble what the user would see after publishing the review. Headers
and footers can be added via the relevant links (either "Add header", "Add
footer", or "Add text" depending on the number of comments), which works a lot
like "Add comment" in the review replies.The individual comments and other states (whether an issue is open, ship it)
are now all "instant apply". The dialog therefore no longer has "Save" and
"Cancel" buttons, but just a "Close" button. Changes to each individual comment
can still be cancelled.
- Clicked "Review" without a pending draft review. Saw the ship-it checkbox and
an open/focused inline editor. - Checked that checking or unchecking "Ship it" would send a request to the
server, editing the review. - Clicked "Review" without a pending draft review. Clicked "Ship it" and
published. Saw a review with "Ship it". - Edited draft reviews, changing comment text and issue state.
- Added and removed headers and footers, checking that everything was saved
correctly, and that when the relevant text was empty/null, that the "Add ..."
links were shown. - Installed the severity extension and verified that changes to the severity
fields were saved when clicking "Close" and "Publish".
| Description | From | Last Updated |
|---|---|---|
|
Do we want to make this an ID, or a class? I assume later on, we'll want links for adding … |
 chipx86
|
|
|
Huh, didn't know about this syntax. Thought you had to do & > |
chipx86
|
|
|
No longer used. |
chipx86
|
|
|
We should put this in a util function in utils/, since do it a few times in this file and … |
chipx86
|
|
|
A lot of this view is similar to BaseCommentView (setting up the editor, needsSave(), save()). Can we make maybe another … |
chipx86
|
|
|
You have both a class and ID with the same name, and we're using the ID in multiple places on … |
chipx86
|
|
|
Blank line between these. |
chipx86
|
-
I could be wrong, but this looks to be breaking/changing an important piece of functionality, which is the support for tying into custom fields provided by extensions.
Previously, a
ReviewDialogCommentHooksubclass could be rendered in and just operate onextraData. We'd compare that when going to save to see if anything there needs saving, and then would save that state. I don't see code that handles this anymore. This will break rbseverity and anything else that makes use of this functionality.Maybe that means those extensions will have to be re-written, but I don't currently see how they can easily do what they did before. I think this code should make it just as easy as before, in some way.
Can you install rbseverity and play around with it and figure out how to keep that working? Maybe instantes of the view get a new option for interfacing with the extraData through a set/get, and then the review dialog can use that to handle saving? (That'd actually end up being a bit nicer than what we have shipping now, I think.) rbseverity could then check for that and use it if provided, or fall back on the current behavior.
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) Do we want to make this an ID, or a class? I assume later on, we'll want links for adding generic comments.
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) Huh, didn't know about this syntax. Thought you had to do
& > -
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1) We should put this in a util function in utils/, since do it a few times in this file and likely will elsewhere in the codebase.
-
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1) A lot of this view is similar to
BaseCommentView(setting up the editor,needsSave(),save()). Can we make maybe another base class that does these common bits so we're not repeating code? -
reviewboard/static/rb/js/views/reviewDialogView.js (Diff revision 1) You have both a class and ID with the same name, and we're using the ID in multiple places on the page. Instead of an ID, we'll need to use a class.
-
Testing Done: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+372 -177) |
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/static/rb/js/utils/textUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/static/rb/js/utils/textUtils.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/static/rb/css/pages/reviews.less reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js