Rewrite the file attachment thumbnail code to be a Backbone view.

Review Request #4050 — Created April 14, 2013 and submitted

Information

Review Board
master

Reviewers

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

daviddavid

Same here with this.$

daviddavid

You don't need the quotes around the object keys here.

daviddavid

These can go inline in the var statement without adding much overhead.

daviddavid

How about using the :empty pseudo-class?

daviddavid

You can use view.$() here too. Same below.

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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?
  2. While it's > 80 columns, I think this would make a lot more sense on one line.
    1. Going to try a different way of wrapping.
  3. Show all issues
    You can use this.$('.file-caption'), etc
  4. Show all issues
    Same here with this.$
  5. reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    You don't need the quotes around the object keys here.
  6. Show all issues
    These can go inline in the var statement without adding much overhead.
  7. Show all issues
    How about using the :empty pseudo-class?
    1. It's not actually empty, so the pseudo-class won't work. We display content.
  8. Show all issues
    You can use view.$() here too. Same below.
  9. 
      
chipx86
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to master (4373ac9)