Summary

Redesign file attachment boxes.

Review Request #7189 — Created April 9, 2015 and submitted April 13, 2015, 3:05 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

7d6496d...

Reviewers

Groups

reviewboard

People

Description

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

Testing Done


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.

Files

Issues

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	April 10, 2015, 2:58 p.m.
Can we make this a constant?	chipx86	April 10, 2015, 2:59 p.m.
So, we have .opacity() for this, which other places use. However, we may want to consider at some point nuking …	chipx86	April 10, 2015, 3 p.m.
Can you stick these back in alphabetical order?	chipx86	April 10, 2015, 3:01 p.m.
What's the 2px?	chipx86	April 10, 2015, 3:03 p.m.
We should have a new top-level @z-index-... variable for any new value, so we can make sure the order exists …	chipx86	April 10, 2015, 3:04 p.m.
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	April 10, 2015, 3:05 p.m.
No need for that trailing space, since the next line has some spaces anyway.	chipx86	April 13, 2015, 3:02 p.m.
The original "spaceless" stuff was to prevent the inline elements from having spaces between tags manifesting in the caption editor, …	chipx86	April 10, 2015, 3:07 p.m.
These lines are a big too long. Can you wrap them?	chipx86	April 10, 2015, 3:09 p.m.
These too.	chipx86	April 10, 2015, 3:09 p.m.
This can be consolidated: '<% if (!loaded) { %>', '<span ...></span>', '<% } else { %>', '<% if (reviewURL) { …	chipx86	April 10, 2015, 3:10 p.m.
Missing a doc block.	chipx86	April 10, 2015, 3:11 p.m.
This is over 79 chars.	chipx86	April 10, 2015, 3:12 p.m.
Indentation mismatch.	chipx86	April 10, 2015, 3:12 p.m.
Trailing blank line.	chipx86	April 10, 2015, 3:12 p.m.
Missing a space at the end of the first line.	chipx86	April 10, 2015, 3:13 p.m.

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

Awesome! Looking great. Can't wait to land this.

reviewboard/attachments/mimetypes.py (Diff revision 1)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Can we make this a constant?
```

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Can you stick these back in alphabetical order?
```

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)

The issue has been dropped. Show all issues

What's the 2px?

david April 10, 2015, 3:13 p.m.

It's to make it fit within the border of the parent element.

chipx86 April 13, 2015, 2:52 p.m.

Maybe add a comment, just to help make that clear down the road when we forget. Or constants.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 1)

The issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

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 issue has been resolved. Show all issues

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)

The issue has been resolved. Show all issues

These lines are a big too long. Can you wrap them?

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1)

The issue has been resolved. Show all issues

These too.

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1)

The issue has been resolved. Show all issues

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)
The issue has been resolved. Show all issues
```
Missing a doc block.
```
reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
This is over 79 chars.
```
reviewboard/templates/reviews/changedesc_file_attachment.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Indentation mismatch.
```
reviewboard/templates/reviews/changedesc_file_attachment.html (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing blank line.
```
reviewboard/webapi/resources/base_file_attachment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a space at the end of the first line.
```

Commit:

81be826def23654430375da562c6565d33bbad59

7d6496d88764ab01a567ea8cef2833d3de581a6a

Diff:

Revision 2 (+425 -474)

Show changes

	reviewboard/attachments/mimetypes.py
	reviewboard/attachments/tests.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/ui/base.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/models/reviewRequestEditorModel.js
	16 more

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

Ship it!

reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
No need for that trailing space, since the next line has some spaces anyway.
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (a4e85e5)