Summary

Changed style of Checklist extension to fit RB colour scheme.

Review Request #5939 — Created June 5, 2014 and submitted July 8, 2014, 6:46 p.m.

Information

Owner

salam0smy

Repository

rb-extension-pack

Branch

Bugs

Depends On

~~5933~~, ~~5932~~

Commit

430f7b4...

Reviewers

Groups

students

People

Description

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

Testing Done

Files

Issues

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	July 1, 2014, 2:11 p.m.
All colors should be defined as constants.	david	July 1, 2014, 2:11 p.m.
You can use LESS' nesting to make the stylesheet rules more closely match the HTML: .checklist_header_btn { float: right; list-style: …	david	July 3, 2014, 1:11 a.m.
These can use nested rules.	david	July 3, 2014, 1:11 a.m.
This can be nested inside the previous block.	david	July 3, 2014, 1:11 a.m.
This can also be nested: ``` .checklist_items > li { ... other rules ... &:hover a.checklist_del_item { ... } }	david	July 3, 2014, 1:11 a.m.
Always put a space after the :. Here and several places below.	david	July 3, 2014, 1:55 a.m.
Should have a blank line between blocks. Here and below.	david	July 3, 2014, 1:55 a.m.
Add a space before the {	david	July 3, 2014, 1:56 a.m.
Should be 2-space indent.	david	July 3, 2014, 1:56 a.m.
Should be 2-space indent.	david	July 3, 2014, 1:56 a.m.
Space before the {	david	July 3, 2014, 1:56 a.m.
Can you define this at the top using a name?	david	July 3, 2014, 1:56 a.m.
Trailing whitespace.	david	July 3, 2014, 1:56 a.m.
Should be 2-space indent.	david	July 3, 2014, 1:56 a.m.
Alignment here is off.	david	July 3, 2014, 1:56 a.m.
Trailing whitespace.	david	July 3, 2014, 1:56 a.m.
There are mysterious spaces at the beginning of each of these lines.	david	July 7, 2014, 1:31 p.m.
Needs a space after the :	david	July 7, 2014, 1:31 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    checklist/checklist/static/css/style.less

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

salam0smy July 1, 2014, 2:11 p.m.

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

Description:

		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

Commit:

1a292536a5520a1594b410b21dcc21edc6de4cc8

2fbaba110accab1ab87e93f82caef960d2af6252

Diff:

Revision 2 (+96 -79)

Show changes

checklist/checklist/static/css/style.less

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)

The issue has been resolved. 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.

checklist/checklist/static/css/style.less (Diff revision 2)
The issue has been resolved. Show all issues
```
All colors should be defined as constants.
```

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

The issue has been resolved. 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: ...;
  }
}

Commit:

2fbaba110accab1ab87e93f82caef960d2af6252

1e50c4b5848e439d85476a4a9441477aa68c6810

Diff:

Revision 3 (+100 -68)

Show changes

checklist/checklist/static/css/style.less

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)

The issue has been resolved. Show all issues

These can use nested rules.

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

The issue has been resolved. Show all issues

This can be nested inside the previous block.

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

The issue has been resolved. Show all issues

This can also be nested:
```

.checklist_items > li {

   ... other rules ...
&:hover a.checklist_del_item {

       ...

   }

}

Change Summary:

Used nested css styling

Commit:

1e50c4b5848e439d85476a4a9441477aa68c6810

fe04c340ab57033d34d3df8b05a5f1a05bcb54b4

Diff:

Revision 4 (+104 -74)

Show changes

checklist/checklist/static/css/style.less

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)
The issue has been resolved. Show all issues
```
Always put a space after the :. Here and several places below.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Should have a blank line between blocks. Here and below.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Add a space before the {
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be 2-space indent.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be 2-space indent.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Space before the {
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you define this at the top using a name?
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be 2-space indent.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Alignment here is off.
```
checklist/checklist/static/css/style.less (Diff revision 4)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```

Change Summary:

Fixed indentation and spacing

Commit:

fe04c340ab57033d34d3df8b05a5f1a05bcb54b4

f74d9929e1c9f9a016f042962095d1e92718f95b

Diff:

Revision 5 (+104 -73)

Show changes

checklist/checklist/static/css/style.less

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)
The issue has been resolved. Show all issues
```
There are mysterious spaces at the beginning of each of these lines.
```

checklist/checklist/static/css/style.less (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a space after the :
```

Commit:

f74d9929e1c9f9a016f042962095d1e92718f95b

430f7b48a64c871ac0ff47e1c85532d6eb91cafd

Diff:

Revision 6 (+104 -73)

Show changes

checklist/checklist/static/css/style.less

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to dev/checklist-extension (2605156)

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/css/style.less



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/css/style.less