Summary

Changed Checklist template format

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

Information

Owner

salam0smy

Repository

rb-extension-pack

Branch

Bugs

Depends On

Blocks

5939

Commit

ac2d7a5...

Reviewers

Groups

students

People

Description

The template has been recreated using cascading format, not a table format. Also, the layout of the extension has be alternated; the input text field was moved to the buttom of the extenson. And icons has been replaced with text links, fits better with RB style.

Testing Done

Run RB with checklist extenion enabled on Chrome and Firefox browser, they both have consistent look.

Files

Issues

Description	From	Last Updated
It would be nice to format this to be more readable: '<div class="checklist_box" id="checklist">', ' <div class="checklist_header">', ' <div class="checklist_title">Checklist</div>', …	david	July 1, 2014, 1:01 p.m.
This could be more efficient as this.$('.checklist_box')	david	July 1, 2014, 1:01 p.m.
Same here with this.$	david	July 1, 2014, 1:01 p.m.
Can you move this comment to be just above the render: function() { line?	david	July 3, 2014, 1:23 a.m.
This doesn't look like it's used.	david	July 3, 2014, 1:23 a.m.
There should be a blank line between methods, and a comment above the method explaining what it does.	david	July 3, 2014, 1:23 a.m.
I'd like some style changes in here. I think it also is nice to toggle the variable first and then …	david	July 3, 2014, 1:23 a.m.
You run this.$('.checklist_box') twice. Can you assign that to a variable at the top of the method and then use …	david	July 3, 2014, 1:23 a.m.
Trailing whitespace.	david	July 3, 2014, 1:23 a.m.
You should assign the selector to a variable and then use that, rather than running it twice.	david	July 3, 2014, 1:23 a.m.
We didn't catch this in the original code, but can you remove the space between function and ()?	david	July 7, 2014, 1:32 p.m.
Can you rename this to checklistTemplate?	david	July 3, 2014, 11:57 a.m.
We didn't catch this in the original code, but can you remove the space between function and ()?	david	July 3, 2014, 11:57 a.m.
Can you use camelCase for the variable names, and prefix any jquery object variable name with $? We also try …	david	July 3, 2014, 11:57 a.m.
Add a space after "if"	david	July 3, 2014, 11:57 a.m.
The comments above each line here don't add anything particularly useful--I think it would be just as clear without them, …	david	July 3, 2014, 11:57 a.m.
This comment isn't necessary.	david	July 3, 2014, 11:57 a.m.
Trailing whitespace.	david	July 3, 2014, 11:57 a.m.
We didn't catch this in the original code, but can you remove the space between function and ()?	david	July 3, 2014, 11:57 a.m.
Should use camelCase, $ prefixes, and a single var statement.	david	July 3, 2014, 11:57 a.m.
Can you remove the space between function and ()?	david	July 3, 2014, 11:58 a.m.
One more spot with "function ("	david	July 3, 2014, 4:26 p.m.
Can you align everything with the (? _.bindAll(this, 'render', ..., 'removeItemDB', ..., 'createNewChecklist', ...);	david	July 7, 2014, 12:26 p.m.
This line shouldn't end with a comma.	david	July 7, 2014, 12:26 p.m.
This line shouldn't end with a comma.	david	July 7, 2014, 12:26 p.m.
This line shouldn't end with a comma.	david	July 7, 2014, 12:26 p.m.
This line shouldn't end with a comma.	david	July 7, 2014, 12:26 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

Description:

~		Changed the checklist html template so that it is in a table formatt for better layout and style options, and added more classes to allow more styling options.
	~	Changed the checklist html template so that it is in a table formatt for better layout and style options, and added more classes to allow more styling options. It allowed me to devide the extension style into header and body.
	+	Also, I encapsulated the template in a div tag to give it more padding to allow the maximize button to show.

Commit:

b2e7ceae3c685012908192f2f8714facb12b2654

892cbb2e0ccd1eb57f2f2d79a8cc9e94e3fd9f91

Diff:

Revision 2 (+6 -3)

Show changes

checklist/checklist/static/js/views/checklistView.js

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

Change Summary:

Changed the minimize size so that the button is in the border of the box

Description:

		Changed the checklist html template so that it is in a table formatt for better layout and style options, and added more classes to allow more styling options. It allowed me to devide the extension style into header and body.
~		Also, I encapsulated the template in a div tag to give it more padding to allow the maximize button to show.
	~	Also, I encapsulated the template in a div tag to give it more padding to allow the maximize button to show.
	+	And changed minimize size so that the button is in the border of the box.

Commit:

892cbb2e0ccd1eb57f2f2d79a8cc9e94e3fd9f91

81fb70c8447ca68e527bb747ca205f7ce873bca4

Diff:

Revision 3 (+8 -5)

Show changes

checklist/checklist/static/js/views/checklistView.js

Added Files:

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

Heh, give up and use tables, eh? :)

Using tables for layout is, at least for me, a last-resort solution. Can we not do this same thing with an ordered (or unordered) list and some CSS magic?

mike_conley June 18, 2014, 7:37 p.m.
```
Any update on this stuff?
```

salam0smy June 19, 2014, 8:34 a.m.

! It can definetily be done with a list and css as you'r saying. I thought it would allow more options or better look.
Should have asked about this first.

salam0smy June 19, 2014, 8:35 a.m.

yes, I'm going to open a hackpad for it before I update the post.

Summary:

Changed Checklist template to be in a table format

Changed Checklist template format

Description:

~		Changed the checklist html template so that it is in a table formatt for better layout and style options, and added more classes to allow more styling options. It allowed me to devide the extension style into header and body.
	~	The template has been recreated using cascading format, not a table format. Also, the layout of the extension has be alternated; the input text field was moved to the buttom of the extenson. And icons has been replaced with text links, fits better with RB style.
-		Also, I encapsulated the template in a div tag to give it more padding to allow the maximize button to show.
-		And changed minimize size so that the button is in the border of the box.

Testing Done:

Run RB with checklist extenion enabled on Chrome and Firefox browser, they both have consistent look.

Commit:

81fb70c8447ca68e527bb747ca205f7ce873bca4

bfcaafbfe930bf5ee068941fd974789a69d21890

Diff:

Revision 4 (+33 -30)

Show changes

checklist/checklist/static/js/views/checklistView.js

Added Files:

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

checklist/checklist/static/js/views/checklistView.js (Diff revision 4)

The issue has been resolved. Show all issues

It would be nice to format this to be more readable:

'<div class="checklist_box" id="checklist">',
' <div class="checklist_header">',
'  <div class="checklist_title">Checklist</div>',
'  <div class="actions">',
'   <ul class="checklist_header_btn">',
'    <li><a id="checklist_toggle_size" class="checklist_min">Min</a></li>',
'    <li><a id="checklist_exit">Close</a></li>',
'   </ul>',
'  </div>',
' </div>',
' <div class="checklist_body">',
'  <ul class="checklist_items"></ul>',
' </div>',
' <div class="checklist_footer>',
'  <input name="checklist_item_description" ',
'         placeholder="Add a new item" />
'  <a id="checklist_clear" href="#">X</a>',
' </div>',
'</div>'


I'd also like you to change the "actions" class to be "checklist_actions" and "checklist_itemDesc" to be "checklist_item_description". You may also want to get rid of the "checklist_min" class because that element already has an ID that you could use for styling.

checklist/checklist/static/js/views/checklistView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This could be more efficient as this.$('.checklist_box')
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Same here with this.$
```

Commit:

bfcaafbfe930bf5ee068941fd974789a69d21890

c7cacabd6a72ee96e91fd0bfbd883d21cf0abe0f

Diff:

Revision 5 (+62 -57)

Show changes

checklist/checklist/static/js/views/checklistView.js

Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

checklist/checklist/static/js/views/checklistView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Can you move this comment to be just above the render: function() { line?
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
This doesn't look like it's used.
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 5)

The issue has been resolved. Show all issues

There should be a blank line between methods, and a comment above the method explaining what it does.

checklist/checklist/static/js/views/checklistView.js (Diff revision 5)

The issue has been resolved. Show all issues

I'd like some style changes in here. I think it also is nice to toggle the variable first and then make the necessary changes to the view. The individual comment above each line aren't particularly necessary (I think the code is pretty self explanatory):

this.isMinimized = !this.isMinimized;

if (this.isMinimized) {
    /* Minimize the view */
    ...
} else {
    /* Maximize the view */
    ...
}

It might also be nice to extract the '25' number into a property (this.CHECKLIST_MINIMIZED_HEIGHT?) and rename this.curHeight to this.savedHeight to make it even more self-explanatory.

checklist/checklist/static/js/views/checklistView.js (Diff revision 5)

The issue has been resolved. Show all issues

You run this.$('.checklist_box') twice. Can you assign that to a variable at the top of the method and then use the variable?

checklist/checklist/static/js/views/checklistView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
You should assign the selector to a variable and then use that, rather than running it twice.
```

Change Summary:

Fixed spacing, added local variables for effeciency, and code change on toggle function

Commit:

c7cacabd6a72ee96e91fd0bfbd883d21cf0abe0f

bd11584a9120c8cc791a430bf14c1f8b1f7d5b00

Diff:

Revision 6 (+66 -59)

Show changes

checklist/checklist/static/js/views/checklistView.js

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
We didn't catch this in the original code, but can you remove the space between function and ()?
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you rename this to checklistTemplate?
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
We didn't catch this in the original code, but can you remove the space between function and ()?
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)

The issue has been resolved. Show all issues

Can you use camelCase for the variable names, and prefix any jquery object variable name with $?
We also try to have only one 'var' statement per method:

var heightChange = 0,
    $checklistBox = this.$('.checklist_box'),
    $sizeTextToggle = this.$('#checklist_toggle_size');

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Add a space after "if"
```

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)

The issue has been resolved. Show all issues

The comments above each line here don't add anything particularly useful--I think it would be just as clear without them, now that we've renamed some of the variables and constants.

checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
This comment isn't necessary.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
We didn't catch this in the original code, but can you remove the space between function and ()?
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Should use camelCase, $ prefixes, and a single var statement.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Can you remove the space between function and ()?
```

Change Summary:

Variables names, and spacing

Commit:

bd11584a9120c8cc791a430bf14c1f8b1f7d5b00

cb5e2af25c480e9eaa2f547555df37791bee9e77

Diff:

Revision 7 (+79 -77)

Show changes

checklist/checklist/static/js/views/checklistView.js

Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

checklist/checklist/static/js/views/checklistView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
One more spot with "function ("
```

Commit:

cb5e2af25c480e9eaa2f547555df37791bee9e77

35596753c7ccc7fdb9b58dc893b7a02da31f840f

Diff:

Revision 8 (+82 -80)

Show changes

checklist/checklist/static/js/views/checklistView.js

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js

checklist/checklist/static/js/views/checklistView.js (Diff revision 8)

The issue has been resolved. Show all issues

Can you align everything with the (?

_.bindAll(this, 'render', ...,
          'removeItemDB', ...,
          'createNewChecklist', ...);

checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
This line shouldn't end with a comma.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
This line shouldn't end with a comma.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
This line shouldn't end with a comma.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
This line shouldn't end with a comma.
```

Change Summary:

Fixed spacing, and deleted extra commas

Commit:

35596753c7ccc7fdb9b58dc893b7a02da31f840f

ac2d7a5f2ced1329e554f6c832fd84e33eb5a0cf

Diff:

Revision 9 (+86 -84)

Show changes

checklist/checklist/static/js/views/checklistView.js

Ship it!

```
Ship It!
```

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

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js