Added javascript to uncollapse message issues before navigating to them. (Would not navigate if collapsed before)

Review Request #2646 — Created Oct. 6, 2011 and discarded

Information

Review Board

Reviewers

Added javascript to uncollapse message issues before navigating to them. (Would not navigate if collapsed before)

-Added .click listener to issue descriptions in table.
-Added an id attribute to each issue in order to be able to locate them using jquery.

Fixes:
-Changed <b> to <strong>
-Added trans blocks around every static piece of text.
-Redistributed the CSS classes based on Dave Druska and Mike Conley's recommendation.
-Alphabetized the ordering of CSS rules.
-Double space indentation for rules.

CSS enhancements to Issue Summary Table.

Issue Summary Table starts collapsed.
Increased anchor width to 100%.
Added :hover coloring.
Added left-most and right-most borders.
Fixed padding on Issue Summary Table.
Removed Table if no issues are found.

Fixes:
Removed table entirely if no issues are found.
Removed table row spacing.
Added blue border color for :hover event.
Removed unnecessary CSS rules.
Linux (Ubuntu 11.04): FF 7.01, Chromium 12.
Description From Last Updated

It seems like the border styles are repeated 3 times. Is there a way to combine these?

DD ddruska

These should be in alphabetical order.

mike_conleymike_conley

alphabetical order

mike_conleymike_conley

alphabetical order

mike_conleymike_conley

alphabetical order

mike_conleymike_conley

Also, these should probably be in translation blocks.

mike_conleymike_conley

Should be ".issue .open"

mike_conleymike_conley

Same as above, ".issue .dropped"

mike_conleymike_conley

Same as above, ".issue .resolved"

mike_conleymike_conley

Two space indentation, as opposed to one

mike_conleymike_conley
DD
  1. 
      
  2. Show all issues
    It seems like the border styles are repeated 3 times. Is there a way to combine these?
    1. One alternative would be to add a class to each right-most and left-most element in the table. This still seems like the cheapest way to address this issue. td`s are picky when it comes to CSS rules.
    2. Mike showed me a way to fix this. Thanks for pointing it out.
  3. 
      
mike_conley
  1. Yazan:
    
    Looks good!  Just a few notes below.
    
    Thanks,
    
    -Mike
  2. Alternatively, perhaps we could have .issue be a class on its own, and .open, .dropped, .resolved be classes too.  Then we could just do:
    
    .issue .status {
      border-right-width: 1px;
    }
    
    .issue .description {
      border-left-width: 1px;
    }
    
    .issue .resolved td {
      /* blah */
    }
    
    etc.
    
    Just an idea - though might be more trouble than it's worth.
    1. I implemented your fix. Was able to remove 4 rules. Thanks =).
  3. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    These should be in alphabetical order.
  4. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
    Show all issues
    alphabetical order
  5. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
     
    Show all issues
    alphabetical order
  6. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
     
     
     
     
    Show all issues
    alphabetical order
  7. I think <strong> now is preferred over <b>
  8. reviewboard/templates/reviews/review_detail.html (Diff revision 1)
     
     
     
     
     
    Show all issues
    Also, these should probably be in translation blocks.
  9. 
      
ME
mike_conley
  1. A couple of tiny problems snuck in, but this is looking pretty good.
    
    We might want to double-check with Christian to ensure that the LessCSS stuff that just went in won't impact this work.
    
    -Mike
  2. Show all issues
    Should be ".issue .open"
    1. .issue.open: if both rules apply to an element.
      .issue .open: element .issue with child .open.
      
      The desired rule is .issue.open.
      
      Quick reference: http://www.quirksmode.org/css/multipleclasses.html
    2. Ah, right you are.  My mistake. :)
    3. Feel free to go ahead and drop these three issues.
  3. Show all issues
    Same as above, ".issue .dropped"
  4. Show all issues
    Same as above, ".issue .resolved"
  5. reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 2)
     
     
     
    Show all issues
    Two space indentation, as opposed to one 
  6. 
      
ME
chipx86
  1. My lesscss changes break this. The .css files are gone. You'll need to sync and move your changes over to the .less files. Also, your change deception shows you haven't merged in master. Make sure the description is updated when you do.
  2. 
      
mike_conley
  1. Yazan:
    
    This looks good - I just have one final thing I found.
    
    -Mike
  2. Hm - it might be better to set the name / id of this element to by something like STRINGPREFIX-comment.id, just for clarity's sake.  Same applies below.
    
    Also, this will help make it so that we don't get into any conflicts if Comment / Screenshot / Attachment issue comments have the same ID # (which is an unfortunate consequence of storing each of those models in different tables).
    1. I know I mentioned this on IRC already, but I wanted to put it here too - I think we already have anchors for comments.  We should just be using those.
  3. 
      
ME
Review request changed
Status:
Discarded
Change Summary:
Moved to r2709.