Enhanced Summary Table

Review Request #2709 — Created Nov. 19, 2011 and submitted

Information

Review Board

Reviewers

Enhanced Summary Table

Added javascript to uncollapse and navigate to selected reviews.
Changed css to less css. Changed review_details.html to reflect javascript and css changes.


Fixes:

Fixed javascript uncollapse bug and added comment.
Fixed lesscss style.
Added an anchor selector to the find statement.
Refactored string concatenation.
Fixed comment style.
Removed white space.
Linux (Ubuntu): Chrome 14

Screenshots

Description From Last Updated

If these are indeed the colors we're using everywhere, they'd be best in base.less.

chipx86chipx86

No blank line here. Same with the spots below. If it's the first thing in an indentation block, it shouldn't …

chipx86chipx86

This can just be: border-spacing: 0;

chipx86chipx86

Can we be more specific about which description class? There may be other elements with this class.

chipx86chipx86

Trailing whitespace. It wasn't immediately obvious to me why we were using the href value and wrapping it in a …

chipx86chipx86

Multi-line comments should be in the form of: /* * Comment line * Comment line */ What I meant before …

chipx86chipx86
chipx86
  1. 
      
  2. reviewboard/htdocs/media/rb/css/reviews.less (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues
    If these are indeed the colors we're using everywhere, they'd be best in base.less.
  3. reviewboard/htdocs/media/rb/css/reviews.less (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line here.
    
    Same with the spots below. If it's the first thing in an indentation block, it shouldn't have a blank line before it.
  4. Show all issues
    This can just be:
    
        border-spacing: 0;
  5. Show all issues
    Can we be more specific about which description class? There may be other elements with this class.
  6. Show all issues
    Trailing whitespace.
    
    It wasn't immediately obvious to me why we were using the href value and wrapping it in a jquery element. I get it now, but it should be documented so it's clear, as it's kind of weird.
    
    We should probably be more specific about which <a> tags, since it's specific ones we care about, and things will get weird if another <a> is in there with an actual URL.
  7. 
      
ME
ME
chipx86
  1. Mostly there, but I think there was a miscommunication on my part about the following.
  2. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
     
     
    Show all issues
    Multi-line comments should be in the form of:
    
    /*
     * Comment line
     * Comment line
     */
    
    What I meant before was that the change should be to document why the $($(..)) was happening, just so it's obvious. Not so much a human-readable description of the operations, but the intent.
  3. reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)
     
     
     
    This wasn't quite what I meant. The original problem remains. I think what you had before was mostly fine, except for the .find("a"), which was wrong. If another <a> is ever added somewhere inside $(this), the code will break. Both old and new code.
    
    Instead, you should be doing $(this).find("a.classname"). I think you could then bring back the older code, which was more readable and less fragile.
    1. The change in code was to make it work, as it was looking for an anchor that did not exist. I will add a class selector to the find statement to make sure the proper anchor is selected. The comment was made to explain the weird string concatenations, as that may not seem clear at first.
  4. 
      
ME
ME
CO
  1. Test
  2. 
      
ME
Review request changed
Status:
Completed
Change Summary:
Pushed to master (cc718fd)