Tweak a bunch of things with file attachments.

Review Request #3569 — Created Nov. 27, 2012 and submitted

Information

Review Board
release-1.7.x

Reviewers

Tweak a bunch of things with file attachments.

This change makes a bunch of tweaks to the file attachments view. Probably some
similar tweaks could be made to screenshots but seeing as they're effectively
deprecated, I didn't see the need.

First of all, I've linked the file's thumbnail to the review UI, if appropriate.
This was probably my #1 annoyance with the new system.

Next, the spacing was pretty ugly. This ended up being the product of several
different issues:
- The filename had way too much spacing, which forced it down into an ugly
  position instead of seeming like a title. I've eliminated the top and bottom
  padding.
- The thumbnail image had a max-height of 6em, and the entire file had a height
  of 13em, which meant that the thumbnail image got scaled down in an ugly way
  (to about 68px instead of 100px) and the caption bar got way too much vertical
  space. To fix this, I made the height of the thumbnail always be 100px and
  eliminated the hard-coded height of the file div, letting it scale to fit its
  children.
- After this, attachments with no caption would end up with a collapsed caption
  area. I've given it an explicit height so it's always pretty.
- The empty captions were pretty crappy. I've made it so if there's no caption,
  it explicitly says "no caption" in grey italic and added some javascript to
  the inlineEditor's "complete" handler to toggle the approprate classes.
Attached several images to a review request and double checked all the spacing.
Saved one caption as empty and reloaded the page to see the "No caption" text.
Edited that caption and saw the style switch back to normal. Removed the caption
and saw it be replaced with the italic "No caption". Verified that both draft
and non-draft captions worked as expected.

Clicked on the thumbnail images to load up the review UI.

Verified appearance in Chrome, Firefox, and IE.

Description From Last Updated

=== What happens if someone has a caption of "No caption" ? Can we instead check the empty-caption class?

chipx86chipx86

Feels like we should be able to condense this. The two things that change are the classes and the text. …

chipx86chipx86

You can now do elif

chipx86chipx86
chipx86
  1. 
      
    1. Oh, what does this look like with other browsers? Curious about Firefox and IE.
    2. It looks the same as the screenshots in both firefox and IE.
  2. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    ===
    
    What happens if someone has a caption of "No caption" ? Can we instead check the empty-caption class?
  3. reviewboard/templates/reviews/review_request_box.html (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Feels like we should be able to condense this. The two things that change are the classes and the text. Maybe instead of all this, we can use {% definevar %} from djblets to set the class and text, and then just output the <a> once.
  4. Show all issues
    You can now do elif
  5. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.7.x (a1e7fef).
Loading...