Summary Table Patch

Review Request #3167 — Created June 30, 2012 and submitted

Information

Review Board

Reviewers

Summary Table Patch:
  -Added issue-link anchor around the issue id column
  -Added reviewer filter
  -Added a "There are no (open|closed|dropped) issues [from reviewer]" message for filters with no results 
  -Restyled "Back to summary table" button and fixed overlay issue for non-review owners
  -Uncollapse the box of the targeted comment if within a collapsed box   
  -Minor optimizations/style fixes/code refactoring

A screenshot of "moving the filters into the table columns" suggestion (ChipX86) is also provided.
Local dev, Chrome

Description From Last Updated

It'd be nicer if this was left-aligned. And actually, maybe we don't even want the headers in this case?

chipx86chipx86

This needs more contrast. It looks like it's just white on yellow with no separation, until you look close. Try …

chipx86chipx86

Not sure these comments are really necessary, but they should at least match the indentation of the variables. Same with …

chipx86chipx86

table_tbody should just be tbody, I think. The underscore doesn't match the style of other variables.

chipx86chipx86

Most of the code generally does: tbody.append($('') .addClass('no-issues') .append($('') .attr....

chipx86chipx86

What is this doing?

chipx86chipx86

Not sure this is necessary.

chipx86chipx86

This will fail. You'll need to use {% static "rb/images/list.png" %}

chipx86chipx86

This is going to introduce 3 new SQL queries. I'd like to avoid that. Instead, process what you need in …

chipx86chipx86

This should be window.location.hash. Will fix this later tonight.

ME medanat
chipx86
  1. 
      
  2. Show all issues
    It'd be nicer if this was left-aligned.
    
    And actually, maybe we don't even want the headers in this case?
  3. Show all issues
    This needs more contrast. It looks like it's just white on yellow with no separation, until you look close. Try a darker border.
  4. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
    Show all issues
    Not sure these comments are really necessary, but they should at least match the indentation of the variables. Same with the ones below.
  5. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    table_tbody should just be tbody, I think. The underscore doesn't match the style of other variables.
  6. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    Most of the code generally does:
    
    tbody.append($('<tr/>')
        .addClass('no-issues')
        .append($('<td/>')
            .attr....
  7. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    What is this doing?
    1. This will scroll down the window to the targeted anchor now that it's box has been uncollapsed.
    2. I think the way it's done is a bit weird. It really should be setting window.location explicitly. Otherwise, it's too much acknowledgement of magic, and I'm not convinced all browsers will treat it correctly.
    3. So one thing I can do is be more explicit and say: window.location = window.location. The window.location method is a very old standard and is available in all browsers.
      
      Another approach could be to measure the offset height and use scrollTop to scroll down to the anchor location. I think the window.location method is much cleaner.
      
      Do you know any specific browsers where the window.location method will not work? 
  8. reviewboard/static/rb/js/reviews.js (Diff revision 1)
     
     
    Show all issues
    Not sure this is necessary.
    1. This brings issueSummaryTableManager into the local scope of the function, making it faster to reference locally. It's a tiny optimization.
    2. I'm all for tiny optimizations when they're in code that needs them, but I don't feel this is one. I'd prefer readability here. It's kind of a slippery slope. It's far more important when we're looping a lot or calling something with great frequency.
  9. Show all issues
    This will fail. You'll need to use {% static "rb/images/list.png" %}
  10. Show all issues
    This is going to introduce 3 new SQL queries. I'd like to avoid that. Instead, process what you need in review_detail and diff and use that variable here.
    
    Also, does this work on the diff viewer?
    1. I'm not 100% sure what you want me to do here.
      
      You want me to say:
      
      {% for comment in entry.all_comments %} ?
      
      Would that work? Would that get the cached comments instead of querying? I think that would be based on r/3160 which has not been pushed yet.
      
      "Also, does this work on the diff viewer?"
      
      You mean would "get_all_comments" work on the diff viewer? "get_all_comments" is a function I wrote that concatenates "comments.all()", "screenshot_comments.all()", and "file_attachment_comments.all()". It should work in the diff viewer.
    2. We don't have an all_comments yet, but there's separate things for diff_comments, screenshot_comments, and file_attachment_comments. That code could be changed to have a 'comments': {} with the subtypes inside, which you could then loop over.
      
      That change has since been pushed to master, so you should be able to migrate to it.
      
      The diff viewer won't have this, though. I think that's okay. I'm not sure we really want the summary table on the diff viewer anyway.
  11. 
      
chipx86
  1. Hey, have you had time to update the diff? Looking to do a 1.7 beta this weekend.
  2. 
      
ME
ME
ME
  1. 
      
  2. reviewboard/static/rb/js/reviews.js (Diff revision 2)
     
     
    Show all issues
    This should be window.location.hash. Will fix this later tonight.
  3. 
      
ME
ME
chipx86
  1. Ship It!
  2. 
      
ME
Review request changed
Status:
Completed
Change Summary:
Pushed to master (60ffbef)