Summary

Added Collapse All button

Review Request #2808 — Created Jan. 21, 2012 and submitted Jan. 26, 2012, 12:19 p.m.

Information

Owner

medanat

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Added permanent Collapse All button instead of a dynamic toggle.

Testing Done

Local machine: Ubuntu/Chromium.

Issues

Description	From	Last Updated
I think we're going to be putting the issue summary table into the details box above the series of review …	mike_conley	Jan. 23, 2012, 10:48 p.m.
Just curious - why aren't we using the alt attributes?	mike_conley	Jan. 23, 2012, 6:40 a.m.

Ship it!

The code repetition makes me cringe a little, not sure how to fix it though.

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 1)

I feel like this should be collapsed into a single function, but can't think of a good way to do it. The javascript might be simpler if you used hiding instead of modifying the html. Although, that would just be creating repetition in the html instead...

ammok Jan. 21, 2012, 6:36 a.m.

Would it make sense to show both "Expand All" and "Collapse All" buttons.
I can see cases where I might have a couple of them collapsed but wish to expand all of them (and it might not be entirely intuitive to click "Collapse All" before clicking "Expand All".

mike_conley Jan. 22, 2012, 1:40 a.m.

Anthony's got a good point.  Can we try this with both the Expand All and Collapse All buttons visible?

Change Summary:

Added permanent Collapse All button instead of a dynamic toggle.

Summary:

-	Added "Collapse All" button.
+	Added Collapse All button

Description:

~		Extended the "expand all" button to toggle between collapse/expand all.
	~	Added permanent Collapse All button instead of a dynamic toggle.

Diff:

Revision 2 (+19 -4)

Show changes

	reviewboard/htdocs/media/rb/js/reviews.js
	reviewboard/templates/reviews/review_detail.html

Yazan:

This looks good.  Good work.  Two questions.  Once resolved...I think you've got my ship-it.

-Mike

reviewboard/htdocs/media/rb/js/reviews.js (Diff revision 2)

Show all issues

I think we're going to be putting the issue summary table into the details box above the series of review boxes...so I'm not sure this will apply...

medanat Jan. 23, 2012, 6:38 a.m.

Since that is the current behaviour of the page, I believe the comment is valid. The current implementation expands and collapses any "box" class element using the generic "collapse" class. If the layout changes, then the implementation of these functions will change along with the comment.

reviewboard/templates/reviews/review_detail.html (Diff revision 2)

Show all issues

Just curious - why aren't we using the alt attributes?

medanat Jan. 23, 2012, 6:41 a.m.

This was the state of the img element before the edit. I do not know why the alt attribute was left empty, or why the CSS was inline. I can fix both issues if there is no real reason for this implementation.

david Jan. 23, 2012, 3:18 p.m.

It's empty because the <a> also has the text. If it was there, screen readers would read this as "Expand All Expand All".

medanat Jan. 23, 2012, 10:38 p.m.

That makes absolute sense. Thanks David. Is there a reason why the styling was done inline?

david Jan. 24, 2012, 4:10 a.m.

I'm not sure why border was included here instead of the stylesheet. The width and height attributes are used for layout.

medanat Jan. 25, 2012, 12:51 a.m.

I think the issues are resolved? I'll keep the border style inline for consistency, and I'll change the comment if the implementation changes.

Ship it!

This looks good to me.  Thanks Yazan! Great work!

-Mike

Status: Closed (submitted)

Change Summary:

Pushed to master (b801d96). Thanks!