[checklist] Improve checklist extension and user interface
Review Request #6969 — Created Feb. 22, 2015 and submitted
Information | |
---|---|
Sunxperous | |
rb-extension-pack | |
7099 | |
8989a11... | |
Reviewers | |
rb-extension-pack, students | |
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
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
Description | From | Last Updated |
---|---|---|
Instead of adding a new class for this, we should just put everything under the #checklist rule. |
|
|
Wrap this to 80 columns. |
|
|
Maybe define constants somewhere for Enter & Esc? |
|
|
Can this be wrapped to 80 columns? |
|
|
Else should go on the same line as the closing } from the previous block. |
|
|
Only one blank line here. |
|
|
This is only used once. Maybe skip the variable definition and just call .remove(this.model)? |
|
|
It looks like removeItemDB is only called from here? How would you feel about inlining the code? |
|
|
"size" is not a valid attribute on a <div>. |
|
|
Can we use <span> instead of <i>? |
|
|
This doesn't seem right. It should be: if (event.keyCode === $.ui.keyCode.ENTER || event.keyCode === 13) { |
|
|
Same here. |
|
|
And here. |
|
Change Summary:
Finalize changes and ready for review.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+215 -192) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+221 -193) |
||||
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) Instead of adding a new class for this, we should just put everything under the
#checklist
rule. -
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 3) Maybe define constants somewhere for Enter & Esc?
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 3) Can this be wrapped to 80 columns?
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 3) Else should go on the same line as the closing } from the previous block.
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 3) 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) It looks like removeItemDB is only called from here? How would you feel about inlining the code?
Change Summary:
Wrap lines to 80 and add keycode constants.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+231 -202) |

-
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: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+225 -195) |

-
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
-
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. -
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
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: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 6 (+229 -202) |

-
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 addbox-shadow
! :D
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+231 -206) |

-
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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+231 -206) |

-
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) "size" is not a valid attribute on a
<div>
. -
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) Can we use
<span>
instead of<i>
? -
checklist/checklist/static/js/views/checklistView.js (Diff revision 8) This doesn't seem right. It should be:
if (event.keyCode === $.ui.keyCode.ENTER || event.keyCode === 13) {
-
-

-
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