[checklist] Allow single level nesting of checklist items

Review Request #7248 — Created April 24, 2015 and updated

Sunxperous
rb-extension-pack
master
7176
7960
b0f24fc...
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.

Loading file attachments...

Description From Last Updated

Can you indent the nested items a bit more? This is a bit too subtle.

daviddavid

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

brenniebrennie

See previous formatting comment. Since we're doing this in two places, should we just have a helper method to generate ...

brenniebrennie

This should be else. You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the ...

brenniebrennie

Since you are returning a mutable object ([]), this must be a function that returns a new default object each ...

brenniebrennie

Needs doc comment.

brenniebrennie

Needs doc comment.

brenniebrennie

Can this be formatted as: this.$el .children('.checklist-content') .find('.checklist-item-move') .show()

brenniebrennie

Can we do: this.$el .children('.checklist-content') .find('.checklist-item-move') .hide() It is easier to read when formatted like this.

brenniebrennie

Needs doc comment.

brenniebrennie

Make sure to remove commented code from final version.

brenniebrennie

Needs a doc comment.

brenniebrennie

Make sure to remove commented code from final version.

brenniebrennie

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.

daviddavid

The r prefix isn't necessary for this because it doesn't have any characters.

daviddavid

Blank line between these.

brenniebrennie

This should be item.startswith(parent_id). It will end up being faster than checking for all possible substring placements of parent_id in ...

brenniebrennie

This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should ...

brenniebrennie

Should use two backquotes for reStructured Text code tags.

brenniebrennie

No blank line here.

brenniebrennie

``checklist_item_id``

brenniebrennie

The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response

brenniebrennie

Same here.

brenniebrennie

I think Djblets has a .transition() mixin. If it does not, drop this and related issues.

brenniebrennie

Same here.

brenniebrennie

This should use a .transform() mixin. if Djblets doesnt define one, drop this issue.

brenniebrennie

Same here.

brenniebrennie

This is pretty important and I feel it should be in the doccomment for the function.

brenniebrennie

Same here

brenniebrennie

Blank line here.

brenniebrennie

Double quotes preferred here.

brenniebrennie

You are doing item.get('id') twice. let's break that out into an itemID variable.

brenniebrennie

Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?

brenniebrennie
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. 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 int to a six.text_type to use the %s formatter; it will automatically convert it for you.

    Also, make sure to use single quotes for strings.

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

  4. checklist/checklist/models.py (Diff revision 1)
     
     

    This should be else.

    You shouldn't do is comparisons with strings and '' is false-y. This also doesn't handle the case for when nest_under is None.

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

  6. Needs doc comment.

  7. Needs doc comment.

  8. Can this be formatted as:

    this.$el
         .children('.checklist-content')
         .find('.checklist-item-move')
         .show()
    
  9. Can we do:

    this.$el
        .children('.checklist-content')
        .find('.checklist-item-move')
        .hide()
    

    It is easier to read when formatted like this.

  10. Needs doc comment.

  11. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     
     
     

    Make sure to remove commented code from final version.

  12. Needs a doc comment.

  13. checklist/checklist/static/js/views/checklistView.js (Diff revision 1)
     
     
     
     
     
     

    Make sure to remove commented code from final version.

  14. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
  1. 
      
  2. 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
    ]
    
  3. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Can you indent the nested items a bit more? This is a bit too subtle.

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

    This should just use a for loop, rather than sticking it in a list comprehension.

  4. The r prefix isn't necessary for this because it doesn't have any characters.

  5. 
      
SU
reviewbot
  1. 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
    
    
  2. 
      
SU
brennie
  1. 
      
  2. checklist/checklist/models.py (Diff revision 6)
     
     
     

    Blank line between these.

  3. 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 of parent_id in item.

  4. checklist/checklist/models.py (Diff revision 6)
     
     

    This method should ensure that nest_under isn't a second level element (i.e., '_' not in nest_under). If so, it should raise an exception.

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

    Should use two backquotes for reStructured Text code tags.

  6. checklist/checklist/models.py (Diff revision 6)
     
     

    No blank line here.

  7. ``checklist_item_id``
    
  8. The exception I mentioned should be caught here. When that occurs, it should return an INVALID_FORM_DATA response

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

  10. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     

    Same here.

  11. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     
     

    This should use a .transform() mixin. if Djblets doesnt define one, drop this issue.

  12. checklist/checklist/static/css/style.less (Diff revision 6)
     
     
     

    Same here.

  13. This is pretty important and I feel it should be in the doccomment for the function.

  14. Blank line here.

  15. Double quotes preferred here.

  16. checklist/checklist/static/js/views/checklistView.js (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     

    You are doing item.get('id') twice. let's break that out into an itemID variable.

  17. Given that we're pulling out itemID, can we rename this to childIDStart or childStartPos ?

  18. 
      
SU
Review request changed

Change Summary:

Resolved Brennie's issues and slightly reduced width of nested items by 4px.

Commit:

-5a969009d563585bfcb9fa8f233411a697b21fd5
+b0f24fc3f77c6b3dcbefb77a8f038a9b8209e16f

Diff:

Revision 7 (+435 -62)

Show changes

reviewbot
  1. 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
    
    
  2. 
      
Loading...