Modernize the display of the "Review request changed" boxes.
Review Request #5499 — Created Feb. 18, 2014 and submitted
The "Review request changed" boxes haven't been touched since they were
first introduced back in 2006 or 2007. Since then, we've added file
attachments, improved the information available on diffs, introduced
Markdown, and added extension-provided fields. All of which the old
boxes did a terrible job at representing.This introduces a complete overhaul of the design and capabilities of
these boxes. Every bit of information is now more detailed, making it
easier for people to see exactly what has changed at a glance.Text area fields, like Description, are now rendered as Markdown, but
presented in a unified diff format. It's easy to see what changed in the
text, and to even see small changes like a corrected word within a line.
This works much like the diff viewer, except not side-by-side.Simpler text fields, like Branch show the before and after, one on top
of the other, helping to see what changed. These are also presented in a
diff-like view, though much smaller. By placing one above the other,
instead of one to the side of the other, it's easier to see what has
changed.Lists share the same design as the text fields, except there's one row
per removed entry and one per added.Diffs are greatly improved. An uploaded diff still shows the revision, a
link to the diff, and the "Show changes" link. Along with that, it now
shows the raw insert/delete count, and a partial file listing, complete
with complexity icons. If the diff hasn't been viewed yet, the
complexity icons just show the proportion of inserts/deletes, but after
it's viewed and the lines are calculated, it'll show the same
information that the diff viewer shows.File attachments now show the thumbnails for the files that were added
or removed. These are just like the ones on the review request file
listing, but without the editing capabilities. They do have reviewing
capabilities, though, so a user can immediately begin reviewing the
added files without going to the main file listing.
Simple text fields (Branch, Summary, Commit, etc.):
- Changed from blank to a value
- Changed from a value to another value
- Changed from a value to blank
- For Commit, I changed values by posting new diffs.
List fields (Depends On, Reviewers):
- Same general tests as with simple text fields
- Deleted multiple items, and saw one per line.
- Added multiple items, and saw one per line.
- Saw the review request summaries for Depends On.
Bugs field:
- Removed/added multiple entries, and saw only one add and one remove line.
The bugs were comma-separated on a single line, which is nicer, since
bug IDs are generally smaller.
Text area fields (Description, Testing Done):
- Entered lots of Markdown, including code samples, links, and images. Saw them
all render. - Changed all those types of content, including changes within code samples
and changes to lines with images. The formatting didn't break, and the
changes were properly represented. - Changed words in a line, and saw the changed regions highlighted.
Screenshot/file attachment captions:
- Opened review requests with caption changes, and saw that they were displayed
correctly, and links all went to the right places.
File attachments:
- Uploaded file attachments. Saw the thumbnails. I couldn't edit them, but I could
review them. - Tested the Review link and the new Comment link.
- For the New Comment link, I created a comment and saw the draft banner. Published
and saw it appear in the review.
Diffs:
- Uploaded a diff and saw it appear with the information and a file listing.
- Checked all the links, including those on the file listing.
- Verified that the order was the same as in the diff viewer, and that file anchors
were correct even when the visible order changed. - Before viewing the diff, I saw that the complexity icons just displayed insert/delete
counts. - After viewing the diff, the icons showed replace lines, and thickness. They were still
generally in the same proportions as the original icons, with the replace just taking
parts away from the insert/delete regions.
Status changes:
- Closed review requests as discarded.
- Closed review requests as submitted.
- Re-opened review requests.
Change Description field:
- Saw the change description text when I entered it.
General:
- Python unit tests pass.
- JavaScript unit tests pass.
Description | From | Last Updated |
---|---|---|
Can you include the IDs in here? (since that's what's displayed in the review request) |
david | |
I think I'd like it if the description box was aligned with the others. The description doesn't really need to … |
david | |
Leading spaces (at least 1 of them) are unnecessary. |
david | |
filename should be escaped here. |
david | |
There's no reason to include extra newlines in the output. |
david | |
These should be escaped, just in case a localization causes issues. |
david | |
The text should be escaped here. |
david | |
This should be escaped. |
david | |
This should be escaped. |
david | |
There's no reason to include extra newlines in the output. |
david | |
Can you move this to be defined just above the for loop? |
david | |
This comment doesn't really add very much. |
david | |
This would be both fewer lines of code and more efficient as a for loop that appended values. |
david | |
This would be both fewer lines of code and more efficient as a for loop that appended values. |
david | |
It was already this way, but this doesn't need the newlines. |
david | |
Why not import these at the top of the file? |
david | |
!== false seems like a funky comparison. Can we either use === true or just use truthiness? Maybe even better, … |
david | |
If options isn't passed in, shouldn't $thumbnail be set to something else? |
david | |
Same question here about canEdit. |
david | |
Should this have a colon, to be consistent? |
david | |
One leading space here is unnecessary. |
david | |
PEP-8 isn't going to like this alignment. Can you put the first arguments on the next line? |
david |
-
-
-
I think I'd like it if the description box was aligned with the others. The description doesn't really need to use the extra width because it's not that wide in the review request box (and no wider than the summary).
"Description" is also missing a colon.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
!== false
seems like a funky comparison. Can we either use=== true
or just use truthiness? Maybe even better, do something likeif (_.result(this.options.canEdit))
? -
-
-
- Change Summary:
-
- Fixed some string escaping. Using Django's
format_html
andformat_html_join
, which intelligently escapes all strings not already marked as safe. - Made the File Attachments and text area fields appear inline, so that they line up with fields like Branch.
- Included review request IDs in the Depends On field.
- Stripped out some spaces and newlines.
- Improved a comment.
- Moved some imports I accidentally left in a function while initially testing some of this.
- Fixed some string escaping. Using Django's
- Bugs:
- Change Summary:
-
Screwed up posting the diff. See my last change description for the details.
- Commit:
-
d744a9c1aa69deac149af195e11529b92977663b8f3229ceda58a8ba5d989c60dae3ea5ea501600e
- Diff:
-
Revision 2 (+949 -289)
- Change Summary:
-
Added colons to all labels.
- Commit:
-
8f3229ceda58a8ba5d989c60dae3ea5ea501600ea2776f749753e9856a3607e86d936beb826ada6c
- Diff:
-
Revision 3 (+949 -289)
- Change Summary:
-
- Removed a leading space in a string.
- Made the pep8 tool a little more happy.
- Commit:
-
a2776f749753e9856a3607e86d936beb826ada6ca2f9c31c14b8cd3a335e8cd635b33d4df97c3978
- Diff:
-
Revision 4 (+954 -291)