Changed style of Checklist extension to fit RB colour scheme.
Review Request #5939 — Created June 5, 2014 and submitted
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 … |
 david
|
|
|
All colors should be defined as constants. |
david
|
|
|
You can use LESS' nesting to make the stylesheet rules more closely match the HTML: .checklist_header_btn { float: right; list-style: … |
david
|
|
|
These can use nested rules. |
david
|
|
|
This can be nested inside the previous block. |
david
|
|
|
This can also be nested: ``` .checklist_items > li { ... other rules ... &:hover a.checklist_del_item { ... } } |
david
|
|
|
Always put a space after the :. Here and several places below. |
david
|
|
|
Should have a blank line between blocks. Here and below. |
david
|
|
|
Add a space before the { |
david
|
|
|
Should be 2-space indent. |
david
|
|
|
Should be 2-space indent. |
david
|
|
|
Space before the { |
david
|
|
|
Can you define this at the top using a name? |
david
|
|
|
Trailing whitespace. |
david
|
|
|
Should be 2-space indent. |
david
|
|
|
Alignment here is off. |
david
|
|
|
Trailing whitespace. |
david
|
|
|
There are mysterious spaces at the beginning of each of these lines. |
david
|
|
|
Needs a space after the : |
david
|
-
This is a review from Review Bot. Tool: Pyflakes Processed Files: Ignored Files: checklist/checklist/static/css/style.less
-
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).
SA
Description: |
|
||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+96 -79) |
||||||||||||||||||||||||||||||
Added Files: |
-
Tool: PEP8 Style Checker Ignored Files: checklist/checklist/static/css/style.less Tool: Pyflakes Ignored Files: checklist/checklist/static/css/style.less
-
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).
-
checklist/checklist/static/css/style.less (Diff revision 2) 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.
-
checklist/checklist/static/css/style.less (Diff revision 2) All colors should be defined as constants.
-
checklist/checklist/static/css/style.less (Diff revision 2) 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: ...; } }
SA
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+100 -68) |
-
Tool: PEP8 Style Checker Ignored Files: checklist/checklist/static/css/style.less Tool: Pyflakes Ignored Files: checklist/checklist/static/css/style.less
-
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.
-
-
checklist/checklist/static/css/style.less (Diff revision 3) This can be nested inside the previous block.
-
checklist/checklist/static/css/style.less (Diff revision 3) This can also be nested:
```
.checklist_items > li {
... other rules ...&:hover a.checklist_del_item {
...
}
}
SA
Change Summary:
Used nested css styling
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+104 -74) |
-
Tool: PEP8 Style Checker Ignored Files: checklist/checklist/static/css/style.less Tool: Pyflakes Ignored Files: checklist/checklist/static/css/style.less
-
-
checklist/checklist/static/css/style.less (Diff revision 4) Always put a space after the :. Here and several places below.
-
checklist/checklist/static/css/style.less (Diff revision 4) Should have a blank line between blocks. Here and below.
-
-
-
-
-
checklist/checklist/static/css/style.less (Diff revision 4) Can you define this at the top using a name?
-
-
-
-
SA
Change Summary:
Fixed indentation and spacing
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+104 -73) |
-
Tool: Pyflakes Ignored Files: checklist/checklist/static/css/style.less Tool: PEP8 Style Checker Ignored Files: checklist/checklist/static/css/style.less
-
This is looking pretty solid now! Just one small thing.
-
checklist/checklist/static/css/style.less (Diff revision 5) There are mysterious spaces at the beginning of each of these lines.
SA
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+104 -73) |
This is a review from Review Bot. Tool: PEP8 Style Checker Processed Files: Ignored Files: checklist/checklist/static/css/style.less