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...
-
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.
-
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'). -
-
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?
-
-
-
-
- Commit:
-
bd458288dee9c9252d685e93e433b8664de6ce7ccde18d5bd66e259b01d08977a3f337b56fcefae6
- 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
- Commit:
-
cde18d5bd66e259b01d08977a3f337b56fcefae63524174c0db7dbcdd6a7fe75c72d908073e647bd
- 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
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