WIP: Checklist Protoype

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

Information

rb-extension-pack

Reviewers

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.


Description From Last Updated

We probably want checkboxes to toggle these.

mike_conleymike_conley

Can probably be replaced with X icons.

mike_conleymike_conley

This can probably be replaced with a simple + icon.

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. Show all issues

    This should be split up:

    tagName: 'li',
    className: 'item',
    
  3. Show all issues

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

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

    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. Show all issues

    Leftover debug output?

  6. Show all issues

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

  7. Show all issues

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

  8. Show all issues

    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. Show all issues

    Multi-line comments should use / /

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

    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. Show all issues

    We probably want checkboxes to toggle these.

  3. Show all issues

    Can probably be replaced with X icons.

  4. Show all issues

    This can probably be replaced with a simple + icon.

  5. Show all issues

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

  6. Show all issues

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

  7. Show all issues

    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. Show all issues

    We could probably replace this with an icon, no?

  9. Show all issues

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

  10. Show all issues

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

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

    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. Show all issues

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

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

  13. Show all issues

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

  14. 
      
chipx86
  1. 
      
  2. Show all issues

    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)
     
     
     
     
    Show all issues

    Two blank lines.

  4. Show all issues

    Spaces inside the / and /

  5. Show all issues

    Space before {

    Same below.

  6. Show all issues

    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. Show all issues

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

  8. Show all issues

    Same with classes. .checklist-item

  9. Show all issues

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

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

    Trailing blank line.

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

    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. Show all issues

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

  13. Show all issues

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

    Make sure to have a space before {.

  14. Show all issues

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

  15. Show all issues

    Same here with variables.

    Also, only one var statement, like:

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

    Two blank lines.

  17. checklist/checklist/views.py (Diff revision 2)
     
     
    Show all issues

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

  18. checklist/setup.py (Diff revision 2)
     
     
     
    Show all issues

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

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

    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. Show all issues

    This should be Checklist without quotes.

  4. Show all issues

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

  5. Show all issues

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

  6. Show all issues

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

  7. checklist/checklist/models.py (Diff revision 5)
     
     
    Show all issues

    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)
     
     
    Show all issues

    This should probably be called "review_request"

  9. checklist/checklist/models.py (Diff revision 5)
     
     
     
     
    Show all issues

    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)
     
     
    Show all issues

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

  11. Show all issues

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