Redesign file attachment boxes.
Review Request #7189 — Created April 9, 2015 and submitted
As we've added features, the design of the file attachment boxes has gotten a
little bit crusty. We ended up in a situation where we had an icon, a
thumbnail, a caption, a filename, some buttons, and a drop-down menu. This was
way overly complicated, and managed to stuff a bunch of UI into a small area,
making even the thumbnails pretty useless.This change re-imagines the file attachment boxes with simplicity as the
primary goal. When looking at the page, the only visible information is the
thumbnail and the caption. The thumbnail is a new "HD" thumbnail which is the
full width of the file attachment box. Hovering the mouse over a file box will
trigger an animation that will scroll across the full thumbnail, which makes it
possible to get a lot more information out of it.Additionally, other commands (such as updating, downloading, adding a comment,
or deleting) are all shown in a menu on the left which is shown while hovering
over the file attachment.Internally, this changes file attachments to always render via the javascript
views. We previously had a wacky scenario where the initial page load would
render the HTML in a django template, and then the javascript code would
"import" existing file attachments. Newly-created file attachments would be
rendered from javascript. This resulted in a duplication of the template data
between django templates and the backbone view, except there wasn't an exact
1:1 parity between the two. Rendering entirely from javascript cleans up the
code significantly.I've posted a video of the animation at
https://www.dropbox.com/s/lyohd4firvby0th/new-file-attachments.mov?dl=0
- Added, deleted, and updated file attachments.
- Tested that animation started and stopped when I expected it to, and that
various edge conditions and mouse trickery didn't result in wacky animation. - Checked links for every command.
- Ran unit tests.
- Ran js-tests.
| Description | From | Last Updated |
|---|---|---|
|
Can you switch this to the #: format, so it'll get picked up by Sphinx when we start doing codebase … |
 chipx86
|
|
|
Can we make this a constant? |
chipx86
|
|
|
So, we have .opacity() for this, which other places use. However, we may want to consider at some point nuking … |
chipx86
|
|
|
Can you stick these back in alphabetical order? |
chipx86
|
|
|
What's the 2px? |
chipx86
|
|
|
We should have a new top-level @z-index-... variable for any new value, so we can make sure the order exists … |
chipx86
|
|
|
Just as an optimization, this could be: fileAttachments = _.map(this.options.editorData.fileAttachments, (this.options.editorData.mutableByUser ? this.reviewRequest.draft.createFileAttachment : this.reviewRequest.createFileAttachment)); Since the value isn't going … |
chipx86
|
|
|
No need for that trailing space, since the next line has some spaces anyway. |
chipx86
|
|
|
The original "spaceless" stuff was to prevent the inline elements from having spaces between tags manifesting in the caption editor, … |
chipx86
|
|
|
These lines are a big too long. Can you wrap them? |
chipx86
|
|
|
These too. |
chipx86
|
|
|
This can be consolidated: '<% if (!loaded) { %>', '<span ...></span>', '<% } else { %>', '<% if (reviewURL) { … |
chipx86
|
|
|
Missing a doc block. |
chipx86
|
|
|
This is over 79 chars. |
chipx86
|
|
|
Indentation mismatch. |
chipx86
|
|
|
Trailing blank line. |
chipx86
|
|
|
Missing a space at the end of the first line. |
chipx86
|
-
Awesome! Looking great. Can't wait to land this.
-
reviewboard/attachments/mimetypes.py (Diff revision 1) Can you switch this to the
#:format, so it'll get picked up by Sphinx when we start doing codebase docs? -
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) So, we have
.opacity()for this, which other places use. However, we may want to consider at some point nuking a bunch of those macros, considering most are pretty supported in the browsers we otherwise support.. No strong opinion on whether it's worth fixing up here. -
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) Can you stick these back in alphabetical order?
-
-
reviewboard/static/rb/css/pages/reviews.less (Diff revision 1) We should have a new top-level
@z-index-...variable for any new value, so we can make sure the order exists in one location. -
reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 1) Just as an optimization, this could be:
fileAttachments = _.map(this.options.editorData.fileAttachments, (this.options.editorData.mutableByUser ? this.reviewRequest.draft.createFileAttachment : this.reviewRequest.createFileAttachment));Since the value isn't going to change within the loop, we can just pre-calculate the function to call.
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) The original "spaceless" stuff was to prevent the inline elements from having spaces between tags manifesting in the caption editor, since it threw off editing and I think spacing a bit. Worth verifying that nothing is broken here in Chrome, Firefox, or IE.
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) These lines are a big too long. Can you wrap them?
-
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) This can be consolidated:
'<% if (!loaded) { %>', '<span ...></span>', '<% } else { %>', '<% if (reviewURL) { %>', '<a href="..."></a>', '<% } %>', '<%= thumbnailHTML %>', '<% } %>' -
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) Missing a doc block.
-
-
reviewboard/templates/reviews/changedesc_file_attachment.html (Diff revision 1) Indentation mismatch.
-
reviewboard/templates/reviews/changedesc_file_attachment.html (Diff revision 1) Trailing blank line.
-
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1) Missing a space at the end of the first line.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+425 -474) |
-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/base_file_attachment.py reviewboard/attachments/mimetypes.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/builtin_fields.py reviewboard/attachments/tests.py reviewboard/reviews/ui/base.py Ignored Files: reviewboard/static/rb/js/resources/models/tests/reviewRequestModelTests.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.js reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/js/resources/models/fileAttachmentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/templates/reviews/parts/file_attachment_thumbnail.html reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/templates/reviews/changedesc_file_attachment.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/base_file_attachment.py reviewboard/attachments/mimetypes.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/builtin_fields.py reviewboard/attachments/tests.py reviewboard/reviews/ui/base.py Ignored Files: reviewboard/static/rb/js/resources/models/tests/reviewRequestModelTests.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.js reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/js/resources/models/fileAttachmentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/templates/reviews/parts/file_attachment_thumbnail.html reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/templates/reviews/changedesc_file_attachment.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/css/defs.less reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js
-
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revisions 1 - 2) No need for that trailing space, since the next line has some spaces anyway.
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/base_file_attachment.py reviewboard/attachments/mimetypes.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/builtin_fields.py reviewboard/attachments/tests.py reviewboard/reviews/ui/base.py Ignored Files: reviewboard/static/rb/js/resources/models/tests/reviewRequestModelTests.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.js reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/js/resources/models/fileAttachmentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/templates/reviews/parts/file_attachment_thumbnail.html reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/templates/reviews/changedesc_file_attachment.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/base_file_attachment.py reviewboard/attachments/mimetypes.py reviewboard/reviews/models/file_attachment_comment.py reviewboard/reviews/builtin_fields.py reviewboard/attachments/tests.py reviewboard/reviews/ui/base.py Ignored Files: reviewboard/static/rb/js/resources/models/tests/reviewRequestModelTests.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/static/rb/js/models/tests/reviewRequestEditorModelTests.js reviewboard/static/rb/js/resources/models/tests/fileAttachmentModelTests.js reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/views/tests/reviewDialogViewTests.js reviewboard/static/rb/js/views/reviewRequestEditorView.js reviewboard/static/rb/js/resources/models/fileAttachmentModel.js reviewboard/static/rb/css/pages/reviews.less reviewboard/templates/reviews/reviewable_page_data.js reviewboard/templates/reviews/parts/file_attachment_thumbnail.html reviewboard/static/rb/js/views/reviewDialogView.js reviewboard/templates/reviews/changedesc_file_attachment.html reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/js/models/reviewRequestEditorModel.js reviewboard/static/rb/js/pages/views/reviewablePageView.js reviewboard/static/rb/js/views/tests/reviewRequestEditorViewTests.js