[checklist] Allow single level nesting of checklist items
Review Request #7248 — Created April 24, 2015 and discarded
| Information | |
|---|---|
| Sunxperous | |
| rb-extension-pack | |
| master | |
|
|
|
| 7960 | |
| Reviewers | |
| rb-extension-pack, students | |
Implements single level nested checklist items in preparation for integration of checklist templates.
Checklist items can now be dragged into another for nesting. Order of checklist items is only updated and maintained upon nesting, while rearranging items within the list is not supported yet.
Dragging a checklist item into another nests it, and then the parent checklist item cannot be dragged. A nested checklist item can also be dragged into another top level checklist item, or back to the main checklist.
Once a top level checklist item contains no nested items, it becomes draggable. Deleting a top level checklist item deletes the nested items as well.
Nested checklist items can be minimized within the parent checklist item.
Basic CRUD of non-parent checklist items remain.
Ensured that CRUD functionality of parent checklist items is as expected - nested items are not affected, except when the parent is deleted.
Ensured proper draggable and droppable, and minimizing functionality depending on the state of each checklist item.
Also ensured that checklist item states persist after a refresh, for new items, parent items and child items.
| Description | From | Last Updated |
|---|---|---|
|
Can you indent the nested items a bit more? This is a bit too subtle. |
 david
|
|
|
Can this be formatted as: new_item_id = ('%s_%s' % (nest_under, parent_item['next_item_id'])) You don't have to cast an int to a … |
 brennie
|
|
|
See previous formatting comment. Since we're doing this in two places, should we just have a helper method to generate … |
brennie
|
|
|
This should be else. You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the … |
brennie
|
|
|
Since you are returning a mutable object ([]), this must be a function that returns a new default object each … |
brennie
|
|
|
Needs doc comment. |
brennie
|
|
|
Needs doc comment. |
brennie
|
|
|
Can this be formatted as: this.$el .children('.checklist-content') .find('.checklist-item-move') .show() |
brennie
|
|
|
Can we do: this.$el .children('.checklist-content') .find('.checklist-item-move') .hide() It is easier to read when formatted like this. |
brennie
|
|
|
Needs doc comment. |
brennie
|
|
|
Make sure to remove commented code from final version. |
brennie
|
|
|
Needs a doc comment. |
brennie
|
|
|
Make sure to remove commented code from final version. |
brennie
|
|
|
Use the preferred list comprehension syntax: [ self.checklist_items.pop(item) for item in list(self.checklist_items.keys()) if parent_id in item ] |
SU Sunxperous | |
|
This should just use a for loop, rather than sticking it in a list comprehension. |
david
|
|
|
The r prefix isn't necessary for this because it doesn't have any characters. |
david
|
|
|
Blank line between these. |
brennie
|
|
|
This should be item.startswith(parent_id). It will end up being faster than checking for all possible substring placements of parent_id in … |
brennie
|
|
|
This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should … |
brennie
|
|
|
Should use two backquotes for reStructured Text code tags. |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
``checklist_item_id`` |
brennie
|
|
|
The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response |
brennie
|
|
|
Same here. |
brennie
|
|
|
I think Djblets has a .transition() mixin. If it does not, drop this and related issues. |
brennie
|
|
|
Same here. |
brennie
|
|
|
This should use a .transform() mixin. if Djblets doesnt define one, drop this issue. |
brennie
|
|
|
Same here. |
brennie
|
|
|
This is pretty important and I feel it should be in the doccomment for the function. |
brennie
|
|
|
Same here |
brennie
|
|
|
Blank line here. |
brennie
|
|
|
Double quotes preferred here. |
brennie
|
|
|
You are doing item.get('id') twice. let's break that out into an itemID variable. |
brennie
|
|
|
Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ? |
brennie
|
-
-
checklist/checklist/models.py (Diff revision 1) Can this be formatted as:
new_item_id = ('%s_%s' % (nest_under, parent_item['next_item_id']))You don't have to cast an
intto asix.text_typeto use the%sformatter; it will automatically convert it for you.Also, make sure to use single quotes for strings.
-
checklist/checklist/models.py (Diff revision 1) See previous formatting comment.
Since we're doing this in two places, should we just have a helper method to generate the next item id?
-
checklist/checklist/models.py (Diff revision 1) This should be
else.You shouldn't do
iscomparisons with strings and''is false-y. This also doesn't handle the case for whennest_under is None. -
checklist/checklist/static/js/models/checklist.js (Diff revision 1) Since you are returning a mutable object (
[]), this must be a function that returns a new default object each time. -
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 1) Can this be formatted as:
this.$el .children('.checklist-content') .find('.checklist-item-move') .show() -
checklist/checklist/static/js/views/checklistView.js (Diff revision 1) Can we do:
this.$el .children('.checklist-content') .find('.checklist-item-move') .hide()It is easier to read when formatted like this.
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 1) Make sure to remove commented code from final version.
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 1) Make sure to remove commented code from final version.
SU
Change Summary:
Reduce the scope of this review request to just nesting of checklist items.
Now fully functional nesting of items.
Touch up docs and comments.
Improve multi-line statements.
Summary: |
|
||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+294 -56) |
-
Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
SU
-
-
checklist/checklist/models.py (Diff revision 2) Use the preferred list comprehension syntax:
[ self.checklist_items.pop(item) for item in list(self.checklist_items.keys()) if parent_id in item ]
SU
Change Summary:
Touch up UI, fix some code
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Branch: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 3 (+358 -49) |
-
Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
SU
Change Summary:
Allow minimizing of nested checklist items within parents
Summary: |
|
||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+433 -62) |
||||||||||||||||||||||||||||||||||||
Added Files: |
-
Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
SU
Change Summary:
Prevent minimized nested items from appearing when editing parent description.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+419 -61) |
-
Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
-
-
-
checklist/checklist/models.py (Diff revision 5) This should just use a for loop, rather than sticking it in a list comprehension.
-
checklist/checklist/resources/checklist_item.py (Diff revision 5) The r prefix isn't necessary for this because it doesn't have any characters.
SU
Change Summary:
Add more indentation to nested items, and fix minor code syntax.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+417 -61) |
-
Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
-
-
-
checklist/checklist/models.py (Diff revision 6) This should be
item.startswith(parent_id). It will end up being faster than checking for all possible substring placements ofparent_idinitem. -
checklist/checklist/models.py (Diff revision 6) This method should ensure that
nest_underisn't a second level element (i.e.,'_' not in nest_under). If so, it should raise an exception. -
checklist/checklist/models.py (Diff revision 6) Should use two backquotes for reStructured Text code tags.
-
-
-
checklist/checklist/resources/checklist_item.py (Diff revision 6) The exception I mentioned should be caught here. When that occurs, it should return an
INVALID_FORM_DATAresponse -
-
checklist/checklist/static/css/style.less (Diff revision 6) I think Djblets has a
.transition()mixin. If it does not, drop this and related issues. -
-
checklist/checklist/static/css/style.less (Diff revision 6) This should use a
.transform()mixin. if Djblets doesnt define one, drop this issue. -
-
checklist/checklist/static/js/models/checklist.js (Diff revision 6) This is pretty important and I feel it should be in the doccomment for the function.
-
-
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) Double quotes preferred here.
-
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) You are doing
item.get('id')twice. let's break that out into anitemIDvariable. -
checklist/checklist/static/js/views/checklistView.js (Diff revision 6) Given that we're pulling out
itemID, can we rename this tochildIDStartorchildStartPos?
SU
Change Summary:
Resolved Brennie's issues and slightly reduced width of nested items by 4px.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+435 -62) |
-
Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js
Tool: PEP8 Style Checker Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js Tool: Pyflakes Processed Files: checklist/checklist/models.py checklist/checklist/resources/checklist_item.py Ignored Files: checklist/checklist/static/js/views/checklistView.js checklist/checklist/static/css/style.less checklist/checklist/static/js/models/checklist.js