- Change Summary:
-
Linked with the Template Hook review request.
- Depends On:
WIP: Checklist Protoype
Review Request #4738 — Created Oct. 13, 2013 and discarded
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_conley | |
Can probably be replaced with X icons. |
mike_conley | |
This can probably be replaced with a simple + icon. |
mike_conley | |
How will this checklist go away if I don't need to refer to it? |
mike_conley | |
The checklist should probably be a floaty thing on top of the page, and not in its own scrollable view … |
chipx86 | |
This should be split up: tagName: 'li', className: 'item', |
david | |
There should be a space between the argument list () and the { |
david | |
This can be simplified and cleaned up a lot by using underscore templates. Something like this: template: _.template([ '<% if … |
david | |
Leftover debug output? |
david | |
Comments should have spaces between the comment markers and the text. Since these are all one-line, you could change them … |
david | |
Space between comment marker and text. Please also capitalize sentences. |
david | |
Backbone views have a this.$el attribute which is already wrapped in jquery. This can probably use .html with a single … |
david | |
Multi-line comments should use / / |
david | |
jquery, underscore and backbone should all be available on all pages in review board. You shouldn't ship them as part … |
david | |
Two blank lines. |
chipx86 | |
Spaces inside the / and / |
chipx86 | |
Space before { Same below. |
chipx86 | |
Margins, paddings, with, right, top, etc. do not use percents. They should be integers. |
chipx86 | |
You're sharing a global ID space. You should always have a checklist_ prefix for IDs. |
chipx86 | |
Same with classes. .checklist-item |
chipx86 | |
CSS classes aren't camelCase. This should be .checklist-del-item |
chipx86 | |
Trailing blank line. |
chipx86 | |
Needs to be 4 spaces. Please go through the rest of the code (and your editor) and make sure 4 … |
chipx86 | |
A general nit that we should be using 4 space indentation throughout your JS files. |
mike_conley | |
Some browsers will break with trailing commas. Make sure you don't have any. |
chipx86 | |
We prefer href="#" over javascript:void(0);, since it degrades somewhat more gracefully. |
mike_conley | |
We could probably replace this with an icon, no? |
mike_conley | |
I think itemID is too generic - we might want to make them more specific to checklists - so checklistItemID … |
mike_conley | |
Again, href="#". Also, the indentation should be fixed here. |
mike_conley | |
No need for the _(this.collection.models). You can just use this.collection.models.each(...) Make sure to have a space before {. |
chipx86 | |
Variable declarations must always be at the top of the function. |
chipx86 | |
Same here with variables. Also, only one var statement, like: var id = ..., item = ...; |
chipx86 | |
I don't believe we need all of this boilerplate in your template since it should already be all available in … |
mike_conley | |
Might as well make this a one-liner, like: <div id="checklist"></div> |
mike_conley | |
Is myChecklist ever going to be used? If not, why do we need it? |
mike_conley | |
Two blank lines. |
chipx86 | |
Python and JavaScript code should always use 4 space indentation, no tabs. |
chipx86 | |
This isn't needed with the new static media setup. |
chipx86 | |
It's not terribly important given that this is WIP, but for future reference, python imports are broken up into 3 … |
david | |
This should be Checklist without quotes. |
david | |
If you correctly define "model" above, you should actually be able to skip defining this. |
david | |
You should add user_id and review_request_id in a @webapi_request_fields decorator. |
david | |
This should defined @webapi_request_fields with anything that you pull out of kwargs. |
david | |
All django models automatically get a field called 'id' which is an auto-incrementing primary key. You don't need this. |
david | |
This should probably be called "review_request" |
david | |
Python code typically separates words with underscores (except in class names, which use CamelCase), so these should be items_counter, checklist_items, … |
david | |
This should probably be called "toggle" instead of "edit", since it doesn't accept a new value. |
david | |
This should look something like this, so that url() will resolve properly for both existing and new checklist items: var … |
david |
-
-
-
-
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; },
-
-
Comments should have spaces between the comment markers and the text. Since these are all one-line, you could change them to //, too.
-
-
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);
-
-
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.
- Change Summary:
-
I fixed the formatting issues like the number of spaces and comment formats. I also replaced most of the render code for ChecklistItem with a template. I added a remove functionality, which would remove checklist items when the X button is clicked.
I tried removing the links for JQuery, Backbone and Underscore, but I get an error during development, so currently I left them in there.
- Depends On:
-
- Diff:
Revision 2 (+239)
- Description:
-
~ WIP for the checklist prototype. Currently, this prototype only allows adding new items, and toggling their status from finished to non-finished by clicking on the items. The (X) button currently does nothing. The prototype doesn't have a tab/minimized view yet. It's instantiated when viewing an uploaded file.
~ 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. The prototype doesn't have a tab/minimized view yet. It's instantiated when viewing an uploaded file.
- - I would like some feedback on the Backbone.js files especially, because this is my first time using that framework.
-
Hey Elaine,
Just took a pass at this, and have a few comments. See below!
-Mike
-
-
-
-
-
-
-
-
I think itemID is too generic - we might want to make them more specific to checklists - so checklistItemID or something.
-
-
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.
-
-
-
-
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.
-
-
-
-
-
-
-
-
-
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?
-
-
No need for the
_(this.collection.models)
. You can just usethis.collection.models.each(...)
Make sure to have a space before
{
. -
-
-
-
-
- Change Summary:
-
I fixed most of the things mentioned in the reviews, such as turning the links on the checklist into icons. I also implemented the minimization/ maximization of the checklist. I also brushed up the javascript and python files; I changed variable names, and fixed the indentations.
Some of the bugs on the front-end that I haven't fixed are:
1) Checklist items with long descriptions go outside the box.
2) The positioning of the elements in the checklist go awry when the browser is resized.
3) The height of the checklist becomes fixed after it is maximized.
4) I'm also planning to remove the ADD button once I introduce the form which would connect the checklist to the back-end. The form can add new items using the Enter key. - Diff:
-
Revision 3 (+351)
- Added Files:
- Change Summary:
-
Removed models.py, urls.py and views.py, which were added to the commit by accident.
- Diff:
-
Revision 4 (+315)
- Change Summary:
-
I am working on the API for the extension. I've uploaded the WebAPIResource subclass and the BaseResource subclass. Currently, I'm still having issues with the url for POST.
- Description:
-
~ 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. The prototype doesn't have a tab/minimized view yet. It's instantiated when viewing an uploaded file.
~ 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.
- Diff:
-
Revision 5 (+511)
-
-
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
-
-
-
-
-
All django models automatically get a field called 'id' which is an auto-incrementing primary key. You don't need this.
-
-
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.
-
-
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 + '/');