[checklist] Improve checklist extension and user interface

Review Request #6969 — Created Feb. 22, 2015 and submitted

Information

rb-extension-pack
8989a11...

Reviewers

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.

daviddavid

Wrap this to 80 columns.

daviddavid

Maybe define constants somewhere for Enter & Esc?

daviddavid

Can this be wrapped to 80 columns?

daviddavid

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

daviddavid

Only one blank line here.

daviddavid

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

daviddavid

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

daviddavid

"size" is not a valid attribute on a <div>.

daviddavid

Can we use <span> instead of <i>?

daviddavid

This doesn't seem right. It should be: if (event.keyCode === $.ui.keyCode.ENTER || event.keyCode === 13) {

daviddavid

Same here.

daviddavid

And here.

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
SU
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Can we maybe split the backend and frontend changes into two separate review requests?

  2. Show all issues

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

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

  3. Show all issues

    Wrap this to 80 columns.

  4. Show all issues

    Maybe define constants somewhere for Enter & Esc?

  5. Show all issues

    Can this be wrapped to 80 columns?

  6. Show all issues

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

  7. Show all issues

    Only one blank line here.

  8. Show all issues

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

  9. Show all issues

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

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

    2. Sure.

  10. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
WU
  1. 
      
  2. If it's just #FFFFFF there isn't a need to use a variable here since css already has white

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

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

    1. Added LESS variables as much as possible, thanks.

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

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

  5. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    "size" is not a valid attribute on a <div>.

  3. Show all issues

    Can we use <span> instead of <i>?

  4. Show all issues

    This doesn't seem right. It should be:

    if (event.keyCode === $.ui.keyCode.ENTER ||
        event.keyCode === 13) {
    
    1. 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.

  5. Show all issues

    Same here.

  6. Show all issues

    And here.

  7. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
SU
Review request changed
Status:
Completed
Change Summary:
Pushed to master (743ada6)