Polish the look and feel of file attachment revisions.
Review Request #6734 — Created Jan. 5, 2015 and submitted
This change does a lot of polishing for the new file attachment revisions
feature. Notably, I was highly dissatisfied with the headers above the files,
which looked cluttered and unaligned. I've moved everything into a single
header box (with a slight gradient), and tried to align things as best as I
could. I've also simplified what gets shown, since we really don't need to say
everything three or more times.While I was doing this I made a couple small cleanup changes to the code to be
a little clearer.
- Created several file attachments with images, text, markdown, and other
files. Tested that things looked good when there was just a single revision
(showing just the caption and the file), and when there were multiple
revisions (looking at either a single one of them, or showing a
diff/comparison where appropriate). - Ran unit tests.
- Ran js-tests.
| Description | From | Last Updated |
|---|---|---|
|
Shouldn't the gettext be for the inner string, inside the <h1>? By having the template live in a gettext call, … |
 chipx86
|
|
|
Given the note in the previous reply about how translators can still screw this up, what if we just had … |
chipx86
|
|
|
Can we move this one up? Nearly all our variables in JavaScript are organized into defined variables followed by undefined … |
chipx86
|
|
|
There's an extra blank line here. |
chipx86
|
|
|
These are pretty similar. Can we reduce this by having the conditionals create the items list,a nd then assemble that … |
chipx86
|
|
|
Same comment here as above. |
chipx86
|
|
|
This can be: {{caption|default:filename}} |
chipx86
|
|
|
Likewise, this can be: {{diff_caption|default:diff_filename}} |
chipx86
|
|
|
Can we move the {% if %} into the colspan value, to reduce lines and simplify this a bit? |
chipx86
|
|
|
Looks like this was accidentally left in. |
chipx86
|
-
Looks good overall. Just a few comments.
Only thing that I'm unsure about is the amount of space taken up by this design. I don't know what to do about that, though.
Wonder how the slider would look centered...
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) Shouldn't the gettext be for the inner string, inside the
<h1>?By having the template live in a gettext call, we run the risk of the HTML being modified to something intentionally or unintentionally dangerous.
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) Can we move this one up? Nearly all our variables in JavaScript are organized into defined variables followed by undefined variables.
You can also use the
!!shortcut, if you wanted:hasDiff = !!this.model.get('diffAgainstFileAttachmentID'). -
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) There's an extra blank line here.
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) These are pretty similar. Can we reduce this by having the conditionals create the items list,a nd then assemble that into a rendered template after?
-
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 1) Same comment here as above.
-
reviewboard/templates/reviews/ui/text.html (Diff revision 1) This can be:
{{caption|default:filename}} -
reviewboard/templates/reviews/ui/text.html (Diff revision 1) Likewise, this can be:
{{diff_caption|default:diff_filename}} -
reviewboard/templates/reviews/ui/text.html (Diff revision 1) Can we move the
{% if %}into the colspan value, to reduce lines and simplify this a bit?
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+186 -99) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
-
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revisions 1 - 2) Given the note in the previous reply about how translators can still screw this up, what if we just had this be
<%- captionText %>and the template is passedgettext('%(captions) (revision %(revision)s)'), with the values provided?
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+193 -99) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
-
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 3) Looks like this was accidentally left in.
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py reviewboard/reviews/ui/base.py reviewboard/reviews/ui/text.py Ignored Files: reviewboard/static/rb/css/pages/text-review-ui.less reviewboard/templates/reviews/ui/text.html reviewboard/static/rb/css/pages/image-review-ui.less reviewboard/static/rb/js/views/textBasedReviewableView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/views/fileAttachmentRevisionSelectorView.js reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js