Changed style of Checklist extension to fit RB colour scheme.

Review Request #5939 — Created June 5, 2014 and submitted

Information

rb-extension-pack
430f7b4...

Reviewers

Made a couple of changes to the style, so that the extension fits with the theme of RB.
The header of the extension is dark-grey which matches the heaer title of RB. Also, the body of the extension now is in light-blue which matches the blue colour in header of RB.
And I added corner bordrs.
Changed the opacity because after I encapsulated the table in a div tag, the opacity doubled.
Changed the width of the input, so that it is centered, with enough padding right and left.
Added more margin on the right, so that the extension is not touching the scrolling bar.

Icons are not used anymore, changed all icons with text.
Remove item button only appears on hover
The over all colors fits better with RB


 

Description From Last Updated

There are numerous styles in here that affect the whole page. This stylesheet should only contain rules that affect elements …

daviddavid

All colors should be defined as constants.

daviddavid

You can use LESS' nesting to make the stylesheet rules more closely match the HTML: .checklist_header_btn { float: right; list-style: …

daviddavid

These can use nested rules.

daviddavid

This can be nested inside the previous block.

daviddavid

This can also be nested: ``` .checklist_items > li { ... other rules ... &:hover a.checklist_del_item { ... } }

daviddavid

Always put a space after the :. Here and several places below.

daviddavid

Should have a blank line between blocks. Here and below.

daviddavid

Add a space before the {

daviddavid

Should be 2-space indent.

daviddavid

Should be 2-space indent.

daviddavid

Space before the {

daviddavid

Can you define this at the top using a name?

daviddavid

Trailing whitespace.

daviddavid

Should be 2-space indent.

daviddavid

Alignment here is off.

daviddavid

Trailing whitespace.

daviddavid

There are mysterious spaces at the beginning of each of these lines.

daviddavid

Needs a space after the :

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
      Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
mike_conley
  1. This is a good start. I'm not sure if the text was transparent before you changed it, but it appears in the after screenshots to be semi-transparent. Doesn't do much for readability, and if you ask me, this whole thing shouldn't need any transparency (text or background).

    1. I agree with you. It was in the previous styling and I went with it. However, it looks better now with not transperancy.

  2. 
      
SA
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
david
  1. Before I review this in detail, there are a few major issues that you need to address.

    In terms of code style: * LESS files should use 2 space indentation. * There should one blank line between blocks. * Always one space between the selector and the { * Always no space between the rule and the :, and one space between the : and the value.

    It would also be nice to be a bit clearer with when you're using IDs and when you're using classes. In general, IDs should be used when the element is effectively singleton, and classes used when there are many (like the checklist items).

  2. checklist/checklist/static/css/style.less (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    There are numerous styles in here that affect the whole page. This stylesheet should only contain rules that affect elements which are part of the checklist.

  3. Show all issues

    All colors should be defined as constants.

  4. checklist/checklist/static/css/style.less (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can use LESS' nesting to make the stylesheet rules more closely match the HTML:

    .checklist_header_btn {
      float: right;
      list-style: none;
      margin: 0px;
    
      li {
        float: left;
      }
    
      li:hover, a.add-item:hover {
        background: ...;
      }
    }
    
  5. 
      
SA
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
david
  1. There's still a lot of inconsistency with the code style. Please refer to my previous review for the summary of how it should look.

  2. checklist/checklist/static/css/style.less (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These can use nested rules.

  3. checklist/checklist/static/css/style.less (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    This can be nested inside the previous block.

  4. checklist/checklist/static/css/style.less (Diff revision 3)
     
     
     
     
    Show all issues

    This can also be nested:

    ```
    .checklist_items > li {
    ... other rules ...

    &:hover a.checklist_del_item {
    ...
    }
    }

  5. 
      
SA
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Always put a space after the :. Here and several places below.

  3. checklist/checklist/static/css/style.less (Diff revision 4)
     
     
     
    Show all issues

    Should have a blank line between blocks. Here and below.

  4. Show all issues

    Add a space before the {

  5. Show all issues

    Should be 2-space indent.

  6. Show all issues

    Should be 2-space indent.

  7. Show all issues

    Space before the {

  8. Show all issues

    Can you define this at the top using a name?

  9. Show all issues

    Trailing whitespace.

  10. Show all issues

    Should be 2-space indent.

  11. checklist/checklist/static/css/style.less (Diff revision 4)
     
     
     
    Show all issues

    Alignment here is off.

  12. Show all issues

    Trailing whitespace.

  13. 
      
SA
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2. 
      
david
  1. This is looking pretty solid now! Just one small thing.

  2. checklist/checklist/static/css/style.less (Diff revision 5)
     
     
     
     
     
    Show all issues

    There are mysterious spaces at the beginning of each of these lines.

  3. 
      
david
  1. 
      
  2. Show all issues

    Needs a space after the :

  3. 
      
SA
david
  1. Ship It!

  2. 
      
SA
Review request changed
Status:
Completed
Change Summary:
Pushed to dev/checklist-extension (2605156)
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        checklist/checklist/static/css/style.less
    
    
  2.