- Download Diff

Summary:

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 Jan. 9, 2012, 9:12 p.m.

Information
Owner:	medanat
Repository:	Review Board
Branch:
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

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.

Testing Done

Linux (Ubuntu 11.04): FF 7.01, Chromium 12.

Issues

Description	From	Last Updated
It seems like the border styles are repeated 3 times. Is there a way to combine these?	DD ddruska	Oct. 8, 2011, noon
These should be in alphabetical order.	mike_conley	Oct. 15, 2011, 11:35 a.m.
alphabetical order	mike_conley	Oct. 15, 2011, 11:35 a.m.
alphabetical order	mike_conley	Oct. 15, 2011, 11:35 a.m.
alphabetical order	mike_conley	Oct. 15, 2011, 11:35 a.m.
Also, these should probably be in translation blocks.	mike_conley	Oct. 15, 2011, 11:35 a.m.
Should be ".issue .open"	mike_conley	Oct. 16, 2011, 6:38 a.m.
Same as above, ".issue .dropped"	mike_conley	Oct. 16, 2011, 6:38 a.m.
Same as above, ".issue .resolved"	mike_conley	Oct. 16, 2011, 6:38 a.m.
Two space indentation, as opposed to one	mike_conley	Oct. 16, 2011, 6:41 a.m.

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

Show all issues

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

medanat Oct. 8, 2011, noon

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.

medanat Oct. 15, 2011, 11:34 a.m.

Mike showed me a way to fix this. Thanks for pointing it out.

Yazan:

Looks good!  Just a few notes below.

Thanks,

-Mike

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

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.

medanat Oct. 15, 2011, 11:36 a.m.

I implemented your fix. Was able to remove 4 rules. Thanks =).

reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
Show all issues
```
These should be in alphabetical order.
```
reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
Show all issues
```
alphabetical order
```
reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
Show all issues
```
alphabetical order
```
reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 1)
Show all issues
```
alphabetical order
```
reviewboard/templates/reviews/review_detail.html (Diff revision 1)
```
I think <strong> now is preferred over <b>
```
reviewboard/templates/reviews/review_detail.html (Diff revision 1)
Show all issues
```
Also, these should probably be in translation blocks.
```

Change Summary:

-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.

Summary:

-	Added javascript to uncollapse issue message box before navigation.
+	Added javascript to uncollapse message issues before navigating to them. (Would not navigate if collapsed before)

Description:

~		Added javascript to uncollapse issue message box before navigation from summary table. (Would not navigate if collapsed before)
	~	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.
	+
	+	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.

Testing Done:

Linux (Ubuntu 11.04): FF 7.01, Chromium 12.

Diff:

Revision 2 (+63 -39)

Show changes

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

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

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

Show all issues

Should be ".issue .open"

medanat Oct. 16, 2011, 6:35 a.m.

.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

mike_conley Oct. 16, 2011, 6:37 a.m.
```
Ah, right you are.  My mistake. :)
```

mike_conley Oct. 16, 2011, 6:38 a.m.

Feel free to go ahead and drop these three issues.

reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 2)
Show all issues
```
Same as above, ".issue .dropped"
```
1. ME
  
  medanat Oct. 16, 2011, 6:35 a.m.
  Same thing.
reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 2)
Show all issues
```
Same as above, ".issue .resolved"
```
1. ME
  
  medanat Oct. 16, 2011, 6:35 a.m.
  Same thing.
reviewboard/htdocs/media/rb/css/reviews.css (Diff revision 2)
Show all issues
```
Two space indentation, as opposed to one 
```
1. ME
  
  medanat Oct. 16, 2011, 6:41 a.m.
  Fixed, thanks.

Change Summary:

CSS rule indentation fix.

Description:

		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.
	~	-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.

Diff:

Revision 3 (+63 -39)

Show changes

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

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.

Yazan:

This looks good - I just have one final thing I found.

-Mike

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

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).

mike_conley Oct. 16, 2011, 7:27 a.m.

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.

Status: Discarded

Change Summary:

Moved to r2709.

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

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

Change Summary:

Summary:

Description:

Testing Done:

Diff:

Change Summary:

Description:

Diff:

Status: Discarded

Change Summary: