[checklist] Allow single level nesting of checklist items
Review Request #7248 — Created April 24, 2015 and discarded
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
|
-
-
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.
-
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?
-
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. -
Since you are returning a mutable object (
[]), this must be a function that returns a new default object each time. -
-
-
Can this be formatted as:
this.$el .children('.checklist-content') .find('.checklist-item-move') .show() -
Can we do:
this.$el .children('.checklist-content') .find('.checklist-item-move') .hide()It is easier to read when formatted like this.
-
-
-
-
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:
-
[WIP] [checklist] Integrate checklist templates into main checklist[WIP] [checklist] Allow single level nesting of checklist items
- Description:
-
~ Implement single level nested checklist items.
~ Implement single level nested checklist items.
Checklist items can now be dragged into another for nesting. Why is this a WIP?
~ Certain features are still in progress, and templates cannot be imported yet. ~ UI still requires minor touching up. - UI also requires minor touching up. - Testing Done:
-
~ nil, there is always something broken at the moment
~ Basic CRUD of checklist items remain.
+ + Dragging a checklist item into another nests it, and 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.
+ + When a top level checklist item contains no nested items, it is draggable.
+ + Ensured that checklist item states persist after a refresh.
- Commit:
-
881d4c41dcb4d20752c901758e9393dd7e1b0617dfeb1f8806dabed3cab213364db59691f63e1425
-
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:
-
Touch up UI, fix some code
- Description:
-
~ Implement single level nested checklist items.
~ Implements single level nested checklist items in preparation for integration of checklist templates.
- Checklist items can now be dragged into another for nesting. ~ Why is this a WIP?
~ UI still requires minor touching up. ~ 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.
+ + Why is this still a WIP?
+ Checking a parent item should check and uncheck all nested items, and vice versa. + Allow minimizing of nested checklist items within parent checklist items. - Testing Done:
-
~ Basic CRUD of checklist items remain.
~ Basic CRUD of non-parent checklist items remain.
~ Dragging a checklist item into another nests it, and the parent checklist item cannot be dragged.
~ Ensured that CRUD functionality of parent checklist items is as expected - nested items are not affected, except when the parent is deleted.
~ A nested checklist item can also be dragged into another top level checklist item, or back to the main checklist.
~ Ensured proper draggable and droppable functionality depending on the state of each checklist item.
~ When a top level checklist item contains no nested items, it is draggable.
~ Also ensured that checklist item states persist after a refresh, for new items, parent items and child items.
- - Ensured that checklist item states persist after a refresh.
- Branch:
-
master
- Commit:
-
dfeb1f8806dabed3cab213364db59691f63e14259024ac0fe419aeef3a1cf64adcc5868aab72f247
-
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:
-
[WIP] [checklist] Allow single level nesting of checklist items[checklist] Allow single level nesting of checklist items
- Description:
-
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.
~ Why is this still a WIP?
~ Nested checklist items can be minimized within the parent checklist item.
- Checking a parent item should check and uncheck all nested items, and vice versa. - Allow minimizing of nested checklist items within parent checklist items. - Testing Done:
-
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 functionality depending on the state of each checklist item.
~ 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.
- Commit:
-
9024ac0fe419aeef3a1cf64adcc5868aab72f247b5a9cdbfd7a5ca0f50f8d5f28c5c592a642a6c4e
- 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:
-
b5a9cdbfd7a5ca0f50f8d5f28c5c592a642a6c4ea1a9c6490ab8c43fb124ab0794e9ce0a02b6cdd5
-
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
SU
- Change Summary:
-
Add more indentation to nested items, and fix minor code syntax.
- Commit:
-
a1a9c6490ab8c43fb124ab0794e9ce0a02b6cdd55a969009d563585bfcb9fa8f233411a697b21fd5
-
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
-
-
-
This should be
item.startswith(parent_id). It will end up being faster than checking for all possible substring placements ofparent_idinitem. -
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. -
-
-
-
The exception I mentioned should be caught here. When that occurs, it should return an
INVALID_FORM_DATAresponse -
-
-
-
-
-
-
-
-
-
-
SU
- Change Summary:
-
Resolved Brennie's issues and slightly reduced width of nested items by 4px.
- Commit:
-
5a969009d563585bfcb9fa8f233411a697b21fd5b0f24fc3f77c6b3dcbefb77a8f038a9b8209e16f
-
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