WIP: Checklist Protoype

Review Request #4738 — Created Oct. 13, 2013 and discarded

LengMalit
rb-extension-pack
reviewboard, students

WIP for the checklist prototype. Currently, this prototype only allows adding new items, toggling their status from finished to non-finished by clicking on the items, and removing items. It can be minimized/maximized. It's instantiated when viewing an uploaded file.

Currently, I am working on the API to bridge the front-end to the back-end.

Manual testing like adding items and clicking them. Tested in Chrome.

Loading file attachments...

Description From Last Updated

We probably want checkboxes to toggle these.

mike_conleymike_conley

This can probably be replaced with a simple + icon.

mike_conleymike_conley

Can probably be replaced with X icons.

mike_conleymike_conley

How will this checklist go away if I don't need to refer to it?

mike_conleymike_conley

The checklist should probably be a floaty thing on top of the page, and not in its own scrollable view ...

chipx86chipx86

This should be split up: tagName: 'li', className: 'item',

daviddavid

There should be a space between the argument list () and the {

daviddavid

This can be simplified and cleaned up a lot by using underscore templates. Something like this: template: _.template([ '<% if ...

daviddavid

Leftover debug output?

daviddavid

Comments should have spaces between the comment markers and the text. Since these are all one-line, you could change them ...

daviddavid

Space between comment marker and text. Please also capitalize sentences.

daviddavid

Backbone views have a this.$el attribute which is already wrapped in jquery. This can probably use .html with a single ...

daviddavid

Multi-line comments should use / /

daviddavid

jquery, underscore and backbone should all be available on all pages in review board. You shouldn't ship them as part ...

daviddavid

Two blank lines.

chipx86chipx86

Spaces inside the / and /

chipx86chipx86

Space before { Same below.

chipx86chipx86

Margins, paddings, with, right, top, etc. do not use percents. They should be integers.

chipx86chipx86

You're sharing a global ID space. You should always have a checklist_ prefix for IDs.

chipx86chipx86

Same with classes. .checklist-item

chipx86chipx86

CSS classes aren't camelCase. This should be .checklist-del-item

chipx86chipx86

Trailing blank line.

chipx86chipx86

Needs to be 4 spaces. Please go through the rest of the code (and your editor) and make sure 4 ...

chipx86chipx86

A general nit that we should be using 4 space indentation throughout your JS files.

mike_conleymike_conley

Some browsers will break with trailing commas. Make sure you don't have any.

chipx86chipx86

We prefer href="#" over javascript:void(0);, since it degrades somewhat more gracefully.

mike_conleymike_conley

We could probably replace this with an icon, no?

mike_conleymike_conley

I think itemID is too generic - we might want to make them more specific to checklists - so checklistItemID ...

mike_conleymike_conley

Again, href="#". Also, the indentation should be fixed here.

mike_conleymike_conley

No need for the _(this.collection.models). You can just use this.collection.models.each(...) Make sure to have a space before {.

chipx86chipx86

Variable declarations must always be at the top of the function.

chipx86chipx86

Same here with variables. Also, only one var statement, like: var id = ..., item = ...;

chipx86chipx86

I don't believe we need all of this boilerplate in your template since it should already be all available in ...

mike_conleymike_conley

Might as well make this a one-liner, like: <div id="checklist"></div>

mike_conleymike_conley

Is myChecklist ever going to be used? If not, why do we need it?

mike_conleymike_conley

Two blank lines.

chipx86chipx86

Python and JavaScript code should always use 4 space indentation, no tabs.

chipx86chipx86

This isn't needed with the new static media setup.

chipx86chipx86

It's not terribly important given that this is WIP, but for future reference, python imports are broken up into 3 ...

daviddavid

This should be Checklist without quotes.

daviddavid

If you correctly define "model" above, you should actually be able to skip defining this.

daviddavid

You should add user_id and review_request_id in a @webapi_request_fields decorator.

daviddavid

This should defined @webapi_request_fields with anything that you pull out of kwargs.

daviddavid

All django models automatically get a field called 'id' which is an auto-incrementing primary key. You don't need this.

daviddavid

This should probably be called "review_request"

daviddavid

Python code typically separates words with underscores (except in class names, which use CamelCase), so these should be items_counter, checklist_items, ...

daviddavid

This should probably be called "toggle" instead of "edit", since it doesn't accept a new value.

daviddavid

This should look something like this, so that url() will resolve properly for both existing and new checklist items: var ...

daviddavid
LE
david
  1. This is a good start. This review is mostly about backbone fundamentals and some high-level coding style.

    Once you do a little cleanup here, I'd like to see you prototype out the backend with models and API.

    A note on indentation:
    - Python is 4 spaces
    - Javascript is also 4 spaces
    - CSS should be 2 spaces
    - HTML should be one (with the added complication that we do funky things re: django template tags)

  2. This should be split up:

    tagName: 'li',
    className: 'item',
    
  3. There should be a space between the argument list () and the {

  4. checklist/checklist/htdocs/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This can be simplified and cleaned up a lot by using underscore templates. Something like this:

    template: _.template([
        '<% if (finished) { %><strike><% } %>',
        '<a class="itemDone" href="#"><%- description %></a>',
        '<% if (finished) { %></strike><% } %>',
        '<a class="delItem" href="#">(X)</a>'
    ].join('')),
    
    render: function() {
        this.$el.html(this.template(this.model.attributes));
        return this;
    },
    
  5. Leftover debug output?

  6. Comments should have spaces between the comment markers and the text. Since these are all one-line, you could change them to //, too.

  7. Space between comment marker and text. Please also capitalize sentences.

  8. Backbone views have a this.$el attribute which is already wrapped in jquery. This can probably use .html with a single string--that way it reduces the number of calls that manipulate the DOM, and using .html means render can be called idempotently.

    You might also want to save the ul element to an attribute, like this:

    this._$ul = this.$('ul');
    

    which would let you do this in appendItem:

    this._$ul.append(checklistItemView.render().el);
    
  9. Multi-line comments should use / /

  10. checklist/checklist/templates/checklist/template.html (Diff revision 1)
     
     
     
     
     
     
     

    jquery, underscore and backbone should all be available on all pages in review board. You shouldn't ship them as part of your extension and you shouldn't need to explicitly load any of them.

  11. 
      
LE
LE
mike_conley
  1. Hey Elaine,

    Just took a pass at this, and have a few comments. See below!

    -Mike

  2. We probably want checkboxes to toggle these.

  3. This can probably be replaced with a simple + icon.

  4. Can probably be replaced with X icons.

  5. How will this checklist go away if I don't need to refer to it?

  6. A general nit that we should be using 4 space indentation throughout your JS files.

  7. We prefer href="#" over javascript:void(0);, since it degrades somewhat more gracefully.

    1. I had this before, but every time I click on any of my links, the page is refreshed and goes to the top.

  8. We could probably replace this with an icon, no?

  9. I think itemID is too generic - we might want to make them more specific to checklists - so checklistItemID or something.

  10. Again, href="#". Also, the indentation should be fixed here.

  11. checklist/checklist/templates/checklist/template.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     

    I don't believe we need all of this boilerplate in your template since it should already be all available in the places where you want your view.

  12. Might as well make this a one-liner, like:

    <div id="checklist"></div>

  13. Is myChecklist ever going to be used? If not, why do we need it?

  14. 
      
chipx86
  1. 
      
  2. The checklist should probably be a floaty thing on top of the page, and not in its own scrollable view taking space away from the diff viewer.

    1. Oh, the scroll is actually part of the image that I posted. This is actually a screencap of a diff, not an actual diff, because I don't have repository on my localhost. =D

  3. checklist/checklist/extension.py (Diff revision 2)
     
     
     
     

    Two blank lines.

  4. Spaces inside the / and /

  5. Space before {

    Same below.

  6. Margins, paddings, with, right, top, etc. do not use percents. They should be integers.

    1. When I turn them to integers, the size becomes fixed. I wanted the checklist to be relative to the size of the browser window.

    2. I guess width and height can be percentages, but margins and paddings should always be explicit values.

  7. You're sharing a global ID space. You should always have a checklist_ prefix for IDs.

  8. Same with classes. .checklist-item

  9. CSS classes aren't camelCase. This should be .checklist-del-item

  10. checklist/checklist/htdocs/css/index.css (Diff revision 2)
     
     
     

    Trailing blank line.

  11. checklist/checklist/htdocs/js/index.js (Diff revision 2)
     
     

    Needs to be 4 spaces. Please go through the rest of the code (and your editor) and make sure 4 space indentation is used only.

    What's this file for, btw?

    1. Oh, it was from before when I was still trying to figure things out. I removed it now.

  12. Some browsers will break with trailing commas. Make sure you don't have any.

  13. No need for the _(this.collection.models). You can just use this.collection.models.each(...)

    Make sure to have a space before {.

  14. Variable declarations must always be at the top of the function.

  15. Same here with variables.

    Also, only one var statement, like:

    var id = ...,
        item = ...;
    
  16. checklist/checklist/views.py (Diff revision 2)
     
     
     
     

    Two blank lines.

  17. checklist/checklist/views.py (Diff revision 2)
     
     

    Python and JavaScript code should always use 4 space indentation, no tabs.

  18. checklist/setup.py (Diff revision 2)
     
     
     

    This isn't needed with the new static media setup.

  19. 
      
LE
LE
LE
david
  1. 
      
  2. checklist/checklist/checklistResource.py (Diff revision 5)
     
     
     
     
     
     
     
     

    It's not terribly important given that this is WIP, but for future reference, python imports are broken up into 3 sections:

    standard library

    other 3rd party

    this module.

    We also alphabetize these. You should also avoid from ... import *, because it can cause weird namespacing problems. You don't use any standard library stuff, so you'd have something like this:

    from django.contrib.auth.models import User
    from django.http import HttpResponse
    from django.shortcuts import render_to_response
    from djblets.webapi.decorators import (webapi_login_required,
                                           webapi_request_fields,
                                           webapi_response_errors)
    from djblets.webapi.resources import WebAPIResource
    from reviewboard.reviews.models import ReviewRequest
    from reviewboard.webapi.base import WebAPIResponse
    
    from checklist.models import Checklist
    
  3. This should be Checklist without quotes.

  4. If you correctly define "model" above, you should actually be able to skip defining this.

  5. You should add user_id and review_request_id in a @webapi_request_fields decorator.

  6. This should defined @webapi_request_fields with anything that you pull out of kwargs.

  7. checklist/checklist/models.py (Diff revision 5)
     
     

    All django models automatically get a field called 'id' which is an auto-incrementing primary key. You don't need this.

  8. checklist/checklist/models.py (Diff revision 5)
     
     

    This should probably be called "review_request"

  9. checklist/checklist/models.py (Diff revision 5)
     
     
     
     

    Python code typically separates words with underscores (except in class names, which use CamelCase), so these should be items_counter, checklist_items, add_item, etc.

  10. checklist/checklist/models.py (Diff revision 5)
     
     

    This should probably be called "toggle" instead of "edit", since it doesn't accept a new value.

  11. This should look something like this, so that url() will resolve properly for both existing and new checklist items:

    var baseURL = '/api/extensions/checklist.extension.Checklist/checklists/';
    
    return this.isNew() ? baseURL : (baseURL + this.id + '/');
    
  12. 
      
LE
Review request changed

Status: Discarded

Change Summary:

I've uploaded new code.
Loading...