Summary

Overrode TogetherJS default css to make the extension look more like Review Board

Review Request #6066 — Created July 5, 2014 and submitted Aug. 5, 2014, 2:31 p.m.

Information

Owner

PeterTran

Repository

rb-extension-pack

Branch

Bugs

Depends On

Commit

5d837cb...

Reviewers

Groups

reviewboard

People

mike_conley

Description

Overrode TogetherJS default css to make the extension look more like Review Board.

Testing Done

Launch 'Chat' by clicking the Chat tab located on the right side of the header bar. The following views on any page illustrate the new changes:

-The right TogetherJS menu should be dark and similar to the header bar.
-The pop-up titled "Invite a friend" should mimic containers in the admin panel.
-Clicking on the first icon in the TogetherJS menu, the secondary menu that pops up mimics the pull down shown the Support tab in the header bar is hovered over.
-Clicking the last icon (Chat) or third icon (Audio Chat) in the TogetherJS menu, the pop-up should also mimic the containers found in the admin panel.

Files

Issues

Description	From	Last Updated
'settings' imported but unused	reviewbot	July 5, 2014, 9:06 a.m.
'patterns' imported but unused	reviewbot	July 5, 2014, 9:06 a.m.
'include' imported but unused	reviewbot	July 5, 2014, 9:06 a.m.
'HeaderDropdownActionHook' imported but unused	reviewbot	July 5, 2014, 9:06 a.m.
Col: 1 W391 blank line at end of file	reviewbot	July 5, 2014, 9:07 a.m.
I believe the problem here is that the static files need to go under a "static" directory under review_together/review_together. So …	mike_conley	July 6, 2014, 12:03 p.m.
I'd rename this to just "review_together.less"	chipx86	July 9, 2014, 5:50 a.m.
There's trailing whitespace. Multi-line comments are in the form of: /* * line * line */	chipx86	July 9, 2014, 5:50 a.m.
This won't work, and will bring on many setups. Instead, you need to use: @import (reference) "@{STATIC_ROOT}rb/css/defs.less";	chipx86	July 9, 2014, 8:09 a.m.
No blank lines between a selector and its rules. Same with all the ones below. (You do want a blank …	chipx86	July 9, 2014, 8:19 a.m.
It's a bit dangerous to force this here. Can you instead try: font-family: inherit;	chipx86	July 9, 2014, 8:19 a.m.
You probably only need the outer rule. Useful tip for CSS: Unless what you've overriding is nesting ID-based selectors, it's …	chipx86	July 9, 2014, 8:20 a.m.
Place the rules in alphabetical order, here and everywhere else.	chipx86	July 9, 2014, 8:20 a.m.
Should also be: font-family: inherit;	chipx86	July 9, 2014, 8:20 a.m.
Use @box-border-color;	chipx86	July 9, 2014, 8:21 a.m.
Can you put one ID per line?	chipx86	July 9, 2014, 8:25 a.m.
Make sure you don't use tabs anywhere.	chipx86	July 9, 2014, 8:11 a.m.
@font-family: inherit. Same everywhere else this appears.	chipx86	July 9, 2014, 8:25 a.m.
Should use: .border-radius(6px);	chipx86	July 9, 2014, 8:26 a.m.
Space before !important	chipx86	July 9, 2014, 8:27 a.m.
All args should be aligned. This should probably be written as: self.script_injection = TemplateHook( self, 'base-scripts-post', template_name='review_together/base.html')	david	July 16, 2014, 9:29 a.m.
Mind fixing the indentation on this while you're in here?	david	July 16, 2014, 9:29 a.m.
There's a bunch of trailing whitespace in this file. Please clean up everything that's shown in red in the diffviewer.	david	July 16, 2014, 9:30 a.m.
Add a blank line between these two.	david	July 16, 2014, 9:30 a.m.
For most of the extensions in rb-extension-pack, we put the static class variables at the top of the class definition. …	mike_conley	July 19, 2014, 10:33 a.m.
Trailing whitespace.	mike_conley	July 19, 2014, 10:34 a.m.
Trailing whitespace.	mike_conley	July 19, 2014, 10:34 a.m.
There should be a space before the {	david	July 19, 2014, 10:34 a.m.
There should be a space before the {	david	July 19, 2014, 10:34 a.m.
Trailing whitespace.	mike_conley	July 19, 2014, 10:34 a.m.
You've been using 2-space indentation thus far - let's stick with that.	mike_conley	July 19, 2014, 10:34 a.m.
You other colors are all lower-cased.	david	July 19, 2014, 10:35 a.m.
Since you use this color in multiple places, mind pulling it out into a variable?	david	July 19, 2014, 10:39 a.m.
Isn't .togetherjs part of the enclosing block?	david	July 19, 2014, 2:05 p.m.
These can be combined.	david	July 19, 2014, 2:05 p.m.
This can be nested inside the above .togetherjs-window block.	david	July 19, 2014, 2:09 p.m.

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/css/reviewboard-style.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/css/reviewboard-style.less

review_together/review_together/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'settings' imported but unused
```
review_together/review_together/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'patterns' imported but unused
```
review_together/review_together/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'include' imported but unused
```
review_together/review_together/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'HeaderDropdownActionHook' imported but unused
```
review_together/review_together/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```

Change Summary:

Removed unused imports

Commit:

7c7793ded91d53945fc470366cfb1c4944844cad

b5cfad1dd7179f878e49d3977a734b25ee2834e0

Groups:

reviewboard

Diff:

Revision 2 (+131 -3)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/css/reviewboard-style.less

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/css/reviewboard-style.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/css/reviewboard-style.less

review_together/review_together/css/reviewboard-style.less (Diff revision 2)

The issue has been resolved. Show all issues

I believe the problem here is that the static files need to go under a "static" directory under review_together/review_together.
So this should be moved to:
review_together/review_together/static/css/reviewboard-style.less.
Our documentation should probably make this clearer - and that might be a good thing for you to patch as well (or documentation files are here: docs/manual/extending/extensions/static-files.rst

Change Summary:

Added import to correctly use LESS variables from reviewboard codebase. [WIP]

Commit:

b5cfad1dd7179f878e49d3977a734b25ee2834e0

b053b2dbe97325f8ef3c945c56b9d95c4a332d36

Diff:

Revision 3 (+133 -3)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/static/css/reviewboard-style.less

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/reviewboard-style.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/reviewboard-style.less

review_together/review_together/extension.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I'd rename this to just "review_together.less"
```
1. chipx86 July 6, 2014, 1:07 p.m.
  Rather, review-together.less.

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

There's trailing whitespace.

Multi-line comments are in the form of:

/*
 * line
 * line
 */

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

This won't work, and will bring on many setups. Instead, you need to use:
@import (reference) "@{STATIC_ROOT}rb/css/defs.less";

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

No blank lines between a selector and its rules. Same with all the ones below. (You do want a blank line between the rules and the next selector, though, but not between rules and a closing brace).

Also, all selectors should be in alphabetical order, with this selector type ordering:

Tag selectors
ID-based selectors
Class-based selectors

The alphabetical ordering is within those groups.

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

It's a bit dangerous to force this here. Can you instead try:

font-family: inherit;

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

You probably only need the outer rule.

Useful tip for CSS: Unless what you've overriding is nesting ID-based selectors, it's better to have each of your ID-based selectors be top-level and not nested.

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

Place the rules in alphabetical order, here and everywhere else.

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Should also be:
font-family: inherit;
```
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Use @box-border-color;
```
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you put one ID per line?
```
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Make sure you don't use tabs anywhere.
```
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
@font-family: inherit. Same everywhere else this appears.
```

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)

The issue has been resolved. Show all issues

Should use:
.border-radius(6px);

review_together/review_together/static/css/reviewboard-style.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Space before !important
```

Change Summary:

Made corrections requested by Christian regarding the naming and styling of the LESS file.

Commit:

b053b2dbe97325f8ef3c945c56b9d95c4a332d36

3d74b8d68fc558b55ab0213596043d95443bfa4a

Diff:

Revision 4 (+124 -3)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/static/css/review-together.less

Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less



Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less

Peter, can you please upload some screenshots of what the TogetherJS UI looks like with this patch applied to this review request? Thanks!

PeterTran July 13, 2014, 6:31 a.m.

Hi Mike,

I just added the screenshots to this review request.

Peter

Change Summary:: Added screenshots.
Added Files:

review_together/review_together/extension.py (Diff revision 4)

The issue has been resolved. Show all issues

All args should be aligned. This should probably be written as:

self.script_injection = TemplateHook(
    self, 'base-scripts-post',
    template_name='review_together/base.html')

review_together/review_together/extension.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Mind fixing the indentation on this while you're in here?
```

review_together/review_together/static/css/review-together.less (Diff revision 4)

The issue has been resolved. Show all issues

There's a bunch of trailing whitespace in this file. Please clean up everything that's shown in red in the diffviewer.

review_together/review_together/static/css/review-together.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```

Can you also clarify in your description what's left to bring this out of "WIP"?

PeterTran July 16, 2014, 9:31 a.m.

Hi David,

I do not have any further changes to make outside of the corrections from the mentors. I think this is complete.

Thanks

Change Summary:

Completed corrections from David.

Description:

~		Overrode TogetherJS default css to make the extension look more like Review Board [WIP]
	~	Overrode TogetherJS default css to make the extension look more like Review Board.

Commit:

3d74b8d68fc558b55ab0213596043d95443bfa4a

f2b24998783c6f928645703ac792e9da209269ab

Diff:

Revision 5 (+130 -7)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/static/css/review-together.less

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less

Just some style nits - otherwise this looks pretty good! I think the next iteration with these fixes should be landable!

Great work,

-Mike

review_together/review_together/extension.py (Diff revision 5)

The issue has been resolved. Show all issues

For most of the extensions in rb-extension-pack, we put the static class variables at the top of the class definition. Can you please move this block above init?

review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
You've been using 2-space indentation thus far - let's stick with that.
```
```
H
```

review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
There should be a space before the {
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
There should be a space before the {
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
You other colors are all lower-cased.
```
review_together/review_together/static/css/review-together.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Since you use this color in multiple places, mind pulling it out into a variable?
```

Commit:

f2b24998783c6f928645703ac792e9da209269ab

ad93f90b3e58c7d5deac7a023b860b8b65963a4a

Diff:

Revision 6 (+133 -8)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/static/css/review-together.less

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less

review_together/review_together/static/css/review-together.less (Diff revision 6)
The issue has been resolved. Show all issues
```
Isn't .togetherjs part of the enclosing block?
```

review_together/review_together/static/css/review-together.less (Diff revision 6)

The issue has been resolved. Show all issues

These can be combined.

review_together/review_together/static/css/review-together.less (Diff revision 6)
The issue has been resolved. Show all issues
```
This can be nested inside the above .togetherjs-window block.
```

Commit:

ad93f90b3e58c7d5deac7a023b860b8b65963a4a

5d837cb0452f7f509473ef81de99e6fb6f7dd67c

Diff:

Revision 7 (+126 -8)

Show changes

	review_together/review_together/extension.py
	review_together/review_together/static/css/review-together.less

Tool: PEP8 Style Checker
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less



Tool: Pyflakes
Processed Files:
    review_together/review_together/extension.py

Ignored Files:
    review_together/review_together/static/css/review-together.less

Ship it!

This looks good to me!

Peter - can you please update the Testing Done section, detailing the bits of the UI you tested with this skin to make sure it worked? Once that's done, I'll go ahead and land this for you.

PE

PeterTran July 29, 2014, 8:01 p.m.
```
Sure thing, Thanks Mike!
```

Testing Done:: ~
N/A
~
Launch 'Chat' by clicking the Chat tab located on the right side of the header bar. The following views on any page illustrate the new changes:
+
+
-The right TogetherJS menu should be dark and similar to the header bar.
+ -The pop-up titled "Invite a friend" should mimic containers in the admin panel.
+ -Clicking on the first icon in the TogetherJS menu, the secondary menu that pops up mimics the pull down shown the Support tab in the header bar is hovered over.
+ -Clicking the last icon (Chat) or third icon (Audio Chat) in the TogetherJS menu, the pop-up should also mimic the containers found in the admin panel.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (b1a86e6)

~		N/A
	~	Launch 'Chat' by clicking the Chat tab located on the right side of the header bar. The following views on any page illustrate the new changes:
	+
	+	-The right TogetherJS menu should be dark and similar to the header bar.
	+	-The pop-up titled "Invite a friend" should mimic containers in the admin panel.
	+	-Clicking on the first icon in the TogetherJS menu, the secondary menu that pops up mimics the pull down shown the Support tab in the header bar is hovered over.
	+	-Clicking the last icon (Chat) or third icon (Audio Chat) in the TogetherJS menu, the pop-up should also mimic the containers found in the admin panel.