Summary

Summary Table Patch

Review Request #3167 — Created June 30, 2012 and submitted July 31, 2012, 3:10 a.m.

Information

Owner

medanat

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

Local dev, Chrome

Screenshots

Reviewer filter

Non-submitter view

No dropped issues

No dropped issues from John

New issues button

Moved filter suggestion

Updated no issues (r2)

Back to issues r2

Files

Issues

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?	chipx86	July 19, 2012, 5:42 a.m.
This needs more contrast. It looks like it's just white on yellow with no separation, until you look close. Try …	chipx86	July 19, 2012, 5:42 a.m.
Not sure these comments are really necessary, but they should at least match the indentation of the variables. Same with …	chipx86	July 19, 2012, 5:39 a.m.
table_tbody should just be tbody, I think. The underscore doesn't match the style of other variables.	chipx86	July 19, 2012, 5:39 a.m.
Most of the code generally does: tbody.append($('') .addClass('no-issues') .append($('') .attr....	chipx86	July 19, 2012, 5:39 a.m.
What is this doing?	chipx86	Aug. 1, 2012, 1:17 p.m.
Not sure this is necessary.	chipx86	July 19, 2012, 5:40 a.m.
This will fail. You'll need to use {% static "rb/images/list.png" %}	chipx86	July 19, 2012, 5:40 a.m.
This is going to introduce 3 new SQL queries. I'd like to avoid that. Instead, process what you need in …	chipx86	July 19, 2012, 5:40 a.m.
This should be window.location.hash. Will fix this later tonight.	ME medanat	July 20, 2012, 5:18 a.m.

No dropped issues

The issue has been resolved. 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?

No dropped issues

The issue has been resolved. 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.

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. 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.

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. Show all issues

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

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. Show all issues

Most of the code generally does:

tbody.append($('<tr/>')
    .addClass('no-issues')
    .append($('<td/>')
        .attr....

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been dropped. Show all issues

What is this doing?

medanat July 1, 2012, 3:50 a.m.

This will scroll down the window to the targeted anchor now that it's box has been uncollapsed.

chipx86 July 6, 2012, 4:32 a.m.

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.

medanat July 19, 2012, 5:34 a.m.

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?

reviewboard/static/rb/js/reviews.js (Diff revision 1)

The issue has been resolved. Show all issues

Not sure this is necessary.

medanat July 1, 2012, 3:50 a.m.

This brings issueSummaryTableManager into the local scope of the function, making it faster to reference locally. It's a tiny optimization.

chipx86 July 6, 2012, 4:32 a.m.

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.

reviewboard/templates/reviews/comment_issue.html (Diff revision 1)
The issue has been resolved. Show all issues
```
This will fail. You'll need to use {% static "rb/images/list.png" %}
```

reviewboard/templates/reviews/review_issue_summary_table.html (Diff revision 1)

The issue has been resolved. 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?

medanat July 1, 2012, 4:20 a.m.

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.

chipx86 July 6, 2012, 4:32 a.m.

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.

Hey, have you had time to update the diff? Looking to do a 1.7 beta this weekend.

Change Summary:

Handled all issues mentioned in review except for explicitly setting window.location.

Added 2 screenshots to show new "Issues" border and left aligned-no header "no issues" message.

Diff:

Revision 2 (+286 -128)

Show changes

	reviewboard/reviews/views.py
	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/images/list.png
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/comment_issue.html
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_issue_summary_table.html
	reviewboard/templates/reviews/review_request_box.html

Screenshots:

Updated no issues (r2)

Back to issues r2

Updated no issues (r2)

Change Summary:: Removed repeated SS.
Screenshots:: Updated no issues (r2)

reviewboard/static/rb/js/reviews.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be window.location.hash. Will fix this later tonight.
```

Change Summary:

Added window prefix to location.hash.

Diff:

Revision 3 (+286 -128)

Show changes

	reviewboard/reviews/views.py
	reviewboard/static/rb/css/reviews.less
	reviewboard/static/rb/images/list.png
	reviewboard/static/rb/js/reviews.js
	reviewboard/templates/reviews/comment_issue.html
	reviewboard/templates/reviews/review_detail.html
	reviewboard/templates/reviews/review_issue_summary_table.html
	reviewboard/templates/reviews/review_request_box.html

Change Summary:: Added list.png
Added Files:

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (60ffbef)