Summary:

[checklist] Improve checklist extension and user interface

Review Request #6969 — Created Feb. 22, 2015 and submitted April 26, 2015, 3:50 p.m.

Information
Owner:	Sunxperous
Repository:	rb-extension-pack
Branch:
Bugs:
Depends On:
Blocks:	7099
Commit:	8989a11...
Reviewers
Groups:	rb-extension-pack, students
People:

Description

This change aims to polish up the existing checklist UI as well as its code base.

On the UI:

Shifted the UI to be fixed at the bottom (better than in the middle of nowhere)
Uses existing Reviewboard icons as buttons (versus textual buttons)
Reworked toggling of the interface (using CSS animation)
Editing an item selects/highlights the entire text (like renaming files)
Ability to cancel edit via Esc key or button
Word wrapping of long item descriptions
Now escapes special characters in the item descriptions
Trims the string before adding or editing, avoids empty descriptions

In the front end:

Removed delete checklist functionality (since new checklist is generated upon refresh)
Moved remove item method from collection view to item view
So that item ids do not have to be stored in DOM
Fixed a bug where editing multiple items only picks the value of the first item being edited
Enforce hyphens for ids and classes, and single quotes for strings

Testing Done

Glass box front end testing on Chrome 40.0.2214. Quick tests for UI on Safari 8.0.2.

Toggling size works
Marking an item as done or undone works
Deleting an item works
Editing an item works
Cancelling item editing works
Adding an item works
Long text is wrapped
Special characters are escaped
Can edit multiple items without conflict
Whitespace in description is trimmed

Files

Issues

Description	From	Last Updated
Instead of adding a new class for this, we should just put everything under the #checklist rule.	david	March 7, 2015, 1 a.m.
Wrap this to 80 columns.	david	March 7, 2015, 12:59 a.m.
Maybe define constants somewhere for Enter & Esc?	david	March 7, 2015, 12:59 a.m.
Can this be wrapped to 80 columns?	david	March 7, 2015, 12:59 a.m.
Else should go on the same line as the closing } from the previous block.	david	March 7, 2015, 12:59 a.m.
Only one blank line here.	david	March 7, 2015, 12:59 a.m.
This is only used once. Maybe skip the variable definition and just call .remove(this.model)?	david	March 7, 2015, 12:59 a.m.
It looks like removeItemDB is only called from here? How would you feel about inlining the code?	david	April 14, 2015, 4 p.m.
"size" is not a valid attribute on a <div>.	david	April 17, 2015, 1:07 a.m.
Can we use <span> instead of <i>?	david	April 17, 2015, 1:08 a.m.
This doesn't seem right. It should be: if (event.keyCode === $.ui.keyCode.ENTER \|\| event.keyCode === 13) {	david	April 17, 2015, 1:09 a.m.
Same here.	david	April 17, 2015, 1:10 a.m.
And here.	david	April 17, 2015, 1:09 a.m.

Tool: Pyflakes
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less



Tool: PEP8 Style Checker
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less

Change Summary:

Added screenshots for comparison.

Added Files:

Change Summary:

Finalize changes and ready for review.

Summary:

-	[WIP] Improve checklist user interface
+	Improve checklist extension and user interface

Description:

~		Touched up a bit of code on the back end, including calling parent shutdown method, and improving returned HTTP status code.
	~	This change aims to polish up the existing checklist UI as well as its code base.

~		On the front end
	~	In the back end:

~		removed delete checklist functionality (useless since a new checklist generates upon refresh)
~		shifted the UI to be fixed at the bottom (better than floating in the middle of nowhere)
	~	Extension now calls parent shutdown method
	~	Improved returned HTTP status codes (200 and 201)
-		reworked minimizing of the interface (with sweet CSS!)

~		What's currently missing or to be done:
	~	On the UI:

~		Ensure new checklist item input to be aligned to the bottom
~		Eliminate as many UI issues stated in the hackpad
	~	Shifted the UI to be fixed at the bottom (better than in the middle of nowhere)
	~	Uses existing Reviewboard icons as buttons (versus textual buttons)
	+	Reworked toggling of the interface (using CSS animation)
	+	Editing an item selects/highlights the entire text (like renaming files)
	+	Ability to cancel edit via Esc key or button
	+	Word wrapping of long item descriptions
	+	Now escapes special characters in the item descriptions
	+	Trims the string before adding or editing, avoids empty descriptions
	+
	+	In the front end:
	+
	+	Removed delete checklist functionality (since new checklist is generated upon refresh)
	+	Moved remove item method from collection view to item view
	+	So that item ids do not have to be stored in DOM
	+	Fixed a bug where editing multiple items only picks the value of the first item being edited
	+	Enforce hyphens for ids and classes, and single quotes for strings

Testing Done:

~		Front end testing on Chrome 40.0.2214.
	~	Glass box front end testing on Chrome 40.0.2214. Quick tests for UI on Safari 8.0.2.
	+
	+	Toggling size works
	+	Marking an item as done or undone works
	+	Deleting an item works
	+	Editing an item works
	+	Cancelling item editing works
	+	Adding an item works
	+	Long text is wrapped
	+	Special characters are escaped
	+	Can edit multiple items without conflict
	+	Whitespace in description is trimmed

Commit:

-	e575a100e14815fcc7c8c8365aac371cd6f943dd
+	a04d09d13773be57304050b577733943cca1f5e9

Diff:

Revision 2 (+215 -192)

Show changes

	checklist/checklist/extension.py
	checklist/checklist/resources.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Change Summary:

Forgot to push string trim code, and removed extra screenshot.

Commit:

-	a04d09d13773be57304050b577733943cca1f5e9
+	95c396d2f3b586122caeb7227482f1b788b5e54c

Diff:

Revision 3 (+221 -193)

Show changes

	checklist/checklist/extension.py
	checklist/checklist/resources.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Removed Files:

Tool: PEP8 Style Checker
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Can we maybe split the backend and frontend changes into two separate review requests?

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

Show all issues

Instead of adding a new class for this, we should just put everything under the #checklist rule.

Sunxperous March 7, 2015, 1:01 a.m.

Placing it under the #checklist rule causes all child elements to be CSS reset due to #checklist * having more priority than the other classes. .checklist * is less specific and hence has less priority.

checklist/checklist/static/js/models/checklistAPI.js (Diff revision 3)
Show all issues
```
Wrap this to 80 columns.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 3)
Show all issues
```
Maybe define constants somewhere for Enter & Esc?
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 3)
Show all issues
```
Can this be wrapped to 80 columns?
```

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

Show all issues

Else should go on the same line as the closing } from the previous block.

checklist/checklist/static/js/views/checklistView.js (Diff revision 3)
Show all issues
```
Only one blank line here.
```

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

Show all issues

This is only used once. Maybe skip the variable definition and just call .remove(this.model)?

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

Show all issues

It looks like removeItemDB is only called from here? How would you feel about inlining the code?

Sunxperous March 7, 2015, 1:01 a.m.

I think the methods involving the server and database can be better written, perhaps inlining should be done in another review request, since this particular chunk is mainly about moving the removeItem methods from the ChecklistView to the ChecklistItemView.

david April 14, 2015, 4:04 p.m.
```
Sure.
```

Change Summary:

Wrap lines to 80 and add keycode constants.

Commit:

-	95c396d2f3b586122caeb7227482f1b788b5e54c
+	71f8d689da69f0de8e314fcbf71de3942d203ef5

Diff:

Revision 4 (+231 -202)

Show changes

	checklist/checklist/extension.py
	checklist/checklist/resources.py
	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: PEP8 Style Checker
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Processed Files:
    checklist/checklist/extension.py
    checklist/checklist/resources.py

Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Change Summary:

Removed backend changes and update comment style.

Description:

		This change aims to polish up the existing checklist UI as well as its code base.

-		In the back end:
-
-		Extension now calls parent shutdown method
-		Improved returned HTTP status codes (200 and 201)
-
		On the UI:

		Shifted the UI to be fixed at the bottom (better than in the middle of nowhere)
		Uses existing Reviewboard icons as buttons (versus textual buttons)
		Reworked toggling of the interface (using CSS animation)
		Editing an item selects/highlights the entire text (like renaming files)
		Ability to cancel edit via Esc key or button
		Word wrapping of long item descriptions
		Now escapes special characters in the item descriptions
		Trims the string before adding or editing, avoids empty descriptions

		In the front end:

		Removed delete checklist functionality (since new checklist is generated upon refresh)
		Moved remove item method from collection view to item view
		So that item ids do not have to be stored in DOM
		Fixed a bug where editing multiple items only picks the value of the first item being edited
		Enforce hyphens for ids and classes, and single quotes for strings

Commit:

-	71f8d689da69f0de8e314fcbf71de3942d203ef5
+	15566137f517c809e6b7c0485a5ab7abc6a4a5d4

Diff:

Revision 5 (+225 -195)

Show changes

	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

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

If it's just #FFFFFF there isn't a need to use a variable here since css already has white

Sunxperous March 14, 2015, 11:29 p.m.

Thanks for pointing this out, using black and white instead of #000 and @white now.

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

If this value is tied to the max-width of .checklist-item-desc on line 103, you might want to make it a variable.

Sunxperous March 14, 2015, 11:29 p.m.

Added LESS variables as much as possible, thanks.

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

I believe that you can simply do if (itemDesc) here since you've already trimmed it

Sunxperous March 14, 2015, 11:29 p.m.

Do you mean if (!itemDesc)? I feel that the current condition is more explicit though.

Change Summary:

Labelled as [checklist].
Fixed Backbone template indentation to be one space.
Fixed edit item not working.
Removed redundant code.
Improved CSS properties with constants.

Summary:

-	Improve checklist extension and user interface
+	[checklist] Improve checklist extension and user interface

Commit:

-	15566137f517c809e6b7c0485a5ab7abc6a4a5d4
+	d18a3f292bd5b954356bdeb8d01909ee22113b57

Diff:

Revision 6 (+229 -202)

Show changes

	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Change Summary:

Use defs.less and add box-shadow! :D

Commit:

-	d18a3f292bd5b954356bdeb8d01909ee22113b57
+	0dd8c14bf7bbde1b2614b9d61b518998429f9501

Diff:

Revision 7 (+231 -206)

Show changes

	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Change Summary:

Added a missing semi-colon.

Commit:

-	0dd8c14bf7bbde1b2614b9d61b518998429f9501
+	8989a1147c74bf85fb7e8e70de65d9b57e0f6ac2

Diff:

Revision 8 (+231 -206)

Show changes

	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
Show all issues
```
"size" is not a valid attribute on a <div>.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
Show all issues
```
Can we use <span> instead of <i>?
```

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

Show all issues

This doesn't seem right. It should be:

if (event.keyCode === $.ui.keyCode.ENTER ||
    event.keyCode === 13) {

Sunxperous April 17, 2015, 1:36 a.m.

This only fails if $.ui.keyCode.ENTER is defined as something else other than 13, although I agree that your suggestion makes things clearer and failproof.

checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
Show all issues
```
Same here.
```
checklist/checklist/static/js/views/checklistView.js (Diff revision 8)
Show all issues
```
And here.
```

Change Summary:

Fix <i to <span and make keyCode conditions clearer.

Diff:

Revision 9 (+232 -206)

Show changes

	checklist/checklist/static/css/style.less
	checklist/checklist/static/js/models/checklistAPI.js
	checklist/checklist/static/js/views/checklistView.js
	checklist/checklist/templates/checklist/template.html

Tool: Pyflakes
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html



Tool: PEP8 Style Checker
Ignored Files:
    checklist/checklist/static/js/views/checklistView.js
    checklist/checklist/static/js/models/checklistAPI.js
    checklist/checklist/static/css/style.less
    checklist/checklist/templates/checklist/template.html

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (743ada6)