Changed Checklist template format
Review Request #5933 — Created June 4, 2014 and submitted
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.
Run RB with checklist extenion enabled on Chrome and Firefox browser, they both have consistent look.
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>', … |
|
|
This could be more efficient as this.$('.checklist_box') |
|
|
Same here with this.$ |
|
|
Can you move this comment to be just above the render: function() { line? |
|
|
This doesn't look like it's used. |
|
|
There should be a blank line between methods, and a comment above the method explaining what it does. |
|
|
I'd like some style changes in here. I think it also is nice to toggle the variable first and then … |
|
|
You run this.$('.checklist_box') twice. Can you assign that to a variable at the top of the method and then use … |
|
|
Trailing whitespace. |
|
|
You should assign the selector to a variable and then use that, rather than running it twice. |
|
|
We didn't catch this in the original code, but can you remove the space between function and ()? |
|
|
Can you rename this to checklistTemplate? |
|
|
We didn't catch this in the original code, but can you remove the space between function and ()? |
|
|
Can you use camelCase for the variable names, and prefix any jquery object variable name with $? We also try … |
|
|
Add a space after "if" |
|
|
The comments above each line here don't add anything particularly useful--I think it would be just as clear without them, … |
|
|
This comment isn't necessary. |
|
|
Trailing whitespace. |
|
|
We didn't catch this in the original code, but can you remove the space between function and ()? |
|
|
Should use camelCase, $ prefixes, and a single var statement. |
|
|
Can you remove the space between function and ()? |
|
|
One more spot with "function (" |
|
|
Can you align everything with the (? _.bindAll(this, 'render', ..., 'removeItemDB', ..., 'createNewChecklist', ...); |
|
|
This line shouldn't end with a comma. |
|
|
This line shouldn't end with a comma. |
|
|
This line shouldn't end with a comma. |
|
|
This line shouldn't end with a comma. |
|

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: Ignored Files: checklist/checklist/static/js/views/checklistView.js
Description: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 2 (+6 -3) |
|||||||||
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: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||
Diff: |
Revision 3 (+8 -5) |
||||||||||||
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?
Summary: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 4 (+33 -30) |
||||||||||||
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) 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) This could be more efficient as
this.$('.checklist_box')
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+62 -57) |

-
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) Can you move this comment to be just above the
render: function() {
line? -
checklist/checklist/static/js/views/checklistView.js (Diff revision 5) This doesn't look like it's used.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 5) 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) 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 renamethis.curHeight
tothis.savedHeight
to make it even more self-explanatory. -
checklist/checklist/static/js/views/checklistView.js (Diff revision 5) 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) 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+66 -59) |

-
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) 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) Can you rename this to checklistTemplate?
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) 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) 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 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) This comment isn't necessary.
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) 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) Should use camelCase, $ prefixes, and a single var statement.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) Can you remove the space between
function
and()
?
Change Summary:
Variables names, and spacing
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+79 -77) |

-
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) One more spot with "function ("
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+82 -80) |

-
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) Can you align everything with the
(
?_.bindAll(this, 'render', ..., 'removeItemDB', ..., 'createNewChecklist', ...);
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) This line shouldn't end with a comma.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) This line shouldn't end with a comma.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) This line shouldn't end with a comma.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) This line shouldn't end with a comma.
Change Summary:
Fixed spacing, and deleted extra commas
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+86 -84) |