-
-
-
-
-
review_together/review_together/extension.py (Diff revision 1) 'HeaderDropdownActionHook' imported but unused
-
review_together/review_together/extension.py (Diff revision 1) Col: 1 W391 blank line at end of file
Overrode TogetherJS default css to make the extension look more like Review Board
Review Request #6066 — Created July 5, 2014 and submitted
Overrode TogetherJS default css to make the extension look more like Review Board.
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.
| Description | From | Last Updated |
|---|---|---|
|
'settings' imported but unused |
 reviewbot
|
|
|
'patterns' imported but unused |
reviewbot
|
|
|
'include' imported but unused |
reviewbot
|
|
|
'HeaderDropdownActionHook' imported but unused |
reviewbot
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
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
|
|
|
I'd rename this to just "review_together.less" |
 chipx86
|
|
|
There's trailing whitespace. Multi-line comments are in the form of: /* * line * line */ |
chipx86
|
|
|
This won't work, and will bring on many setups. Instead, you need to use: @import (reference) "@{STATIC_ROOT}rb/css/defs.less"; |
chipx86
|
|
|
No blank lines between a selector and its rules. Same with all the ones below. (You do want a blank … |
chipx86
|
|
|
It's a bit dangerous to force this here. Can you instead try: font-family: inherit; |
chipx86
|
|
|
You probably only need the outer rule. Useful tip for CSS: Unless what you've overriding is nesting ID-based selectors, it's … |
chipx86
|
|
|
Place the rules in alphabetical order, here and everywhere else. |
chipx86
|
|
|
Should also be: font-family: inherit; |
chipx86
|
|
|
Use @box-border-color; |
chipx86
|
|
|
Can you put one ID per line? |
chipx86
|
|
|
Make sure you don't use tabs anywhere. |
chipx86
|
|
|
@font-family: inherit. Same everywhere else this appears. |
chipx86
|
|
|
Should use: .border-radius(6px); |
chipx86
|
|
|
Space before !important |
chipx86
|
|
|
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
|
|
|
Mind fixing the indentation on this while you're in here? |
david
|
|
|
There's a bunch of trailing whitespace in this file. Please clean up everything that's shown in red in the diffviewer. |
david
|
|
|
Add a blank line between these two. |
david
|
|
|
For most of the extensions in rb-extension-pack, we put the static class variables at the top of the class definition. … |
mike_conley
|
|
|
Trailing whitespace. |
mike_conley
|
|
|
Trailing whitespace. |
mike_conley
|
|
|
There should be a space before the { |
david
|
|
|
There should be a space before the { |
david
|
|
|
Trailing whitespace. |
mike_conley
|
|
|
You've been using 2-space indentation thus far - let's stick with that. |
mike_conley
|
|
|
You other colors are all lower-cased. |
david
|
|
|
Since you use this color in multiple places, mind pulling it out into a variable? |
david
|
|
|
Isn't .togetherjs part of the enclosing block? |
david
|
|
|
These can be combined. |
david
|
|
|
This can be nested inside the above .togetherjs-window block. |
david
|
PE
Change Summary:
Removed unused imports
Commit: |
|
||||
|---|---|---|---|---|---|
Groups: |
|
||||
Diff: |
Revision 2 (+131 -3) |
-
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) 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
PE
Change Summary:
Added import to correctly use LESS variables from reviewboard codebase. [WIP]
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+133 -3) |
-
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) I'd rename this to just "review_together.less"
-
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) 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) 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) 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) 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) 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) Place the rules in alphabetical order, here and everywhere else.
-
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Should also be:
font-family: inherit; -
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Use
@box-border-color; -
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Can you put one ID per line?
-
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Make sure you don't use tabs anywhere.
-
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) @font-family: inherit. Same everywhere else this appears. -
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Should use:
.border-radius(6px); -
review_together/review_together/static/css/reviewboard-style.less (Diff revision 3) Space before
!important
PE
Change Summary:
Made corrections requested by Christian regarding the naming and styling of the LESS file.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+124 -3) |
-
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!
-
-
review_together/review_together/extension.py (Diff revision 4) 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) Mind fixing the indentation on this while you're in here?
-
review_together/review_together/static/css/review-together.less (Diff revision 4) 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) Add a blank line between these two.
PE
Change Summary:
Completed corrections from David.
Description: |
|
||||||
|---|---|---|---|---|---|---|---|
Commit: |
|
||||||
Diff: |
Revision 5 (+130 -7) |
-
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) 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) Trailing whitespace.
-
review_together/review_together/static/css/review-together.less (Diff revision 5) Trailing whitespace.
-
review_together/review_together/static/css/review-together.less (Diff revision 5) Trailing whitespace.
-
review_together/review_together/static/css/review-together.less (Diff revision 5) 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) There should be a space before the {
-
review_together/review_together/static/css/review-together.less (Diff revision 5) There should be a space before the {
-
review_together/review_together/static/css/review-together.less (Diff revision 5) You other colors are all lower-cased.
-
review_together/review_together/static/css/review-together.less (Diff revision 5) Since you use this color in multiple places, mind pulling it out into a variable?
PE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+133 -8) |
-
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) Isn't .togetherjs part of the enclosing block?
-
review_together/review_together/static/css/review-together.less (Diff revision 6) These can be combined.
-
review_together/review_together/static/css/review-together.less (Diff revision 6) This can be nested inside the above .togetherjs-window block.
PE
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+126 -8) |
-
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
-
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
Testing Done: |
|
|---|
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