Rewrite the file attachment thumbnail code to be a Backbone view.
Review Request #4050 — Created April 14, 2013 and submitted
Rewrite the file attachment thumbnail code to be a Backbone view. The new FileAttachmentThumbnail view can be used with pre-rendered file attachment thumbnails (loaded in from the review request template) and also with newly uploaded files. It also replaces the old file attachment thumbnail placeholder code. It recognizes when we haven't yet finished an upload of a file, and will show a spinner in that case, eventually replacing it with the contents when it's loaded.
Tested existing thumbnails with published captions and draft captions. Tested uploading files through drag-and-drop. Tested uploading files through Add File, with and without a caption. Tested changing captions. Tested deleting draft file attachments. Tested deleting published file attachments. Unit tests pass.
| Description | From | Last Updated |
|---|---|---|
|
You can use this.$('.file-caption'), etc |
 david
|
|
|
Same here with this.$ |
david
|
|
|
You don't need the quotes around the object keys here. |
david
|
|
|
These can go inline in the var statement without adding much overhead. |
david
|
|
|
How about using the :empty pseudo-class? |
david
|
|
|
You can use view.$() here too. Same below. |
david
|
-
One thing I'd vaguely noticed was that file attachments that were on existing loaded pages and file attachments that got created from javascript didn't have identical HTML. Can you verify that it's the same after your change?
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) While it's > 80 columns, I think this would make a lot more sense on one line.
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) You can use this.$('.file-caption'), etc -
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) Same here with this.$
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) You don't need the quotes around the object keys here.
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) These can go inline in the var statement without adding much overhead.
-
reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1) How about using the :empty pseudo-class?
-
reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js (Diff revision 1) You can use view.$() here too. Same below.
Change Summary:
* Switched to this.$(...) * Changed wrapping for a line. * Moved some variable initialization inline. * Removed quotes on on() handlers.
Diff: |
Revision 2 (+628 -217) |
|---|
-
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/settings.py Ignored Files: reviewboard/static/rb/js/reviews.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/views/dndUploaderView.js
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: reviewboard/settings.py Ignored Files: reviewboard/static/rb/js/reviews.js reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js reviewboard/templates/reviews/review_request_box.html reviewboard/static/rb/js/views/tests/fileAttachmentThumbnailViewTests.js reviewboard/static/rb/js/views/dndUploaderView.js