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

Review Request #6066 — Created July 5, 2014 and submitted

Information

rb-extension-pack
5d837cb...

Reviewers

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

reviewbotreviewbot

'patterns' imported but unused

reviewbotreviewbot

'include' imported but unused

reviewbotreviewbot

'HeaderDropdownActionHook' imported but unused

reviewbotreviewbot

Col: 1 W391 blank line at end of file

reviewbotreviewbot

I believe the problem here is that the static files need to go under a "static" directory under review_together/review_together. So …

mike_conleymike_conley

I'd rename this to just "review_together.less"

chipx86chipx86

There's trailing whitespace. Multi-line comments are in the form of: /* * line * line */

chipx86chipx86

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

chipx86chipx86

No blank lines between a selector and its rules. Same with all the ones below. (You do want a blank …

chipx86chipx86

It's a bit dangerous to force this here. Can you instead try: font-family: inherit;

chipx86chipx86

You probably only need the outer rule. Useful tip for CSS: Unless what you've overriding is nesting ID-based selectors, it's …

chipx86chipx86

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

chipx86chipx86

Should also be: font-family: inherit;

chipx86chipx86

Use @box-border-color;

chipx86chipx86

Can you put one ID per line?

chipx86chipx86

Make sure you don't use tabs anywhere.

chipx86chipx86

@font-family: inherit. Same everywhere else this appears.

chipx86chipx86

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

chipx86chipx86

Space before !important

chipx86chipx86

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

daviddavid

Mind fixing the indentation on this while you're in here?

daviddavid

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

daviddavid

Add a blank line between these two.

daviddavid

For most of the extensions in rb-extension-pack, we put the static class variables at the top of the class definition. …

mike_conleymike_conley

Trailing whitespace.

mike_conleymike_conley

Trailing whitespace.

mike_conleymike_conley

There should be a space before the {

daviddavid

There should be a space before the {

daviddavid

Trailing whitespace.

mike_conleymike_conley

You've been using 2-space indentation thus far - let's stick with that.

mike_conleymike_conley

You other colors are all lower-cased.

daviddavid

Since you use this color in multiple places, mind pulling it out into a variable?

daviddavid

Isn't .togetherjs part of the enclosing block?

daviddavid

These can be combined.

daviddavid

This can be nested inside the above .togetherjs-window block.

daviddavid
reviewbot
  1. 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
    
    
  2. Show all issues
     'settings' imported but unused
    
  3. Show all issues
     'patterns' imported but unused
    
  4. Show all issues
     'include' imported but unused
    
  5. Show all issues
     'HeaderDropdownActionHook' imported but unused
    
  6. Show all issues
    Col: 1
     W391 blank line at end of file
    
  7. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. 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

  3. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    I'd rename this to just "review_together.less"

    1. Rather, review-together.less.

  3. Show all issues

    There's trailing whitespace.

    Multi-line comments are in the form of:

    /*
     * line
     * line
     */
    
  4. 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";
    
  5. 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.

  6. Show all issues

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

    font-family: inherit;
    
  7. 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.

  8. Show all issues

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

  9. Show all issues

    Should also be:

    font-family: inherit;
    
  10. Show all issues

    Use @box-border-color;

  11. Show all issues

    Can you put one ID per line?

  12. Show all issues

    Make sure you don't use tabs anywhere.

  13. Show all issues

    @font-family: inherit. Same everywhere else this appears.

  14. Show all issues

    Should use:

    .border-radius(6px);
    
  15. Show all issues

    Space before !important

  16. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. Peter, can you please upload some screenshots of what the TogetherJS UI looks like with this patch applied to this review request? Thanks!

    1. Hi Mike,

      I just added the screenshots to this review request.

      Peter

  2. 
      
PE
david
  1. 
      
  2. 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')
    
  3. review_together/review_together/extension.py (Diff revision 4)
     
     
     
     
    Show all issues

    Mind fixing the indentation on this while you're in here?

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

  5. Show all issues

    Add a blank line between these two.

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

    1. Hi David,

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

      Thanks

  2. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. Just some style nits - otherwise this looks pretty good! I think the next iteration with these fixes should be landable!

    Great work,

    -Mike

  2. review_together/review_together/extension.py (Diff revision 5)
     
     
     
     
     
     
     
    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?

  3. Show all issues

    Trailing whitespace.

  4. Show all issues

    Trailing whitespace.

  5. Show all issues

    Trailing whitespace.

  6. Show all issues

    You've been using 2-space indentation thus far - let's stick with that.

  7. H

david
  1. 
      
  2. Show all issues

    There should be a space before the {

  3. Show all issues

    There should be a space before the {

  4. Show all issues

    You other colors are all lower-cased.

  5. Show all issues

    Since you use this color in multiple places, mind pulling it out into a variable?

  6. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Isn't .togetherjs part of the enclosing block?

  3. Show all issues

    These can be combined.

  4. Show all issues

    This can be nested inside the above .togetherjs-window block.

  5. 
      
PE
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 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.

    1. Sure thing, Thanks Mike!

  2. 
      
PE
david
  1. Ship It!

  2. 
      
PE
Review request changed
Status:
Completed
Change Summary:
Pushed to master (b1a86e6)