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

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

PeterTran
rb-extension-pack
5d837cb...
reviewboard
mike_conley

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.  'settings' imported but unused
    
  3.  'patterns' imported but unused
    
  4.  'include' imported but unused
    
  5.  'HeaderDropdownActionHook' imported but unused
    
  6. 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. 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. I'd rename this to just "review_together.less"

    1. Rather, review-together.less.

  3. There's trailing whitespace.

    Multi-line comments are in the form of:

    /*
     * line
     * line
     */
    
  4. This won't work, and will bring on many setups. Instead, you need to use:

    @import (reference) "@{STATIC_ROOT}rb/css/defs.less";
    
  5. 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. It's a bit dangerous to force this here. Can you instead try:

    font-family: inherit;
    
  7. 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. Place the rules in alphabetical order, here and everywhere else.

  9. Should also be:

    font-family: inherit;
    
  10. Can you put one ID per line?

  11. Make sure you don't use tabs anywhere.

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

  13. Should use:

    .border-radius(6px);
    
  14. Space before !important

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

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

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

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

    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. You've been using 2-space indentation thus far - let's stick with that.

  4. H

david
  1. 
      
  2. There should be a space before the {

  3. There should be a space before the {

  4. You other colors are all lower-cased.

  5. 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. Isn't .togetherjs part of the enclosing block?

  3. These can be combined.

  4. 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: Closed (submitted)

Change Summary:

Pushed to master (b1a86e6)
Loading...