• 
      

    Allow items in configform lists to fade in/out when added/removed.

    Review Request #6151 — Created July 27, 2014 and submitted

    Information

    Djblets
    release-0.8.x
    762a1cd...

    Reviewers

    This adds an 'animateItems' option that, if set, will fade in newly
    added items and fade out removed items. This can be controlled by the
    'animate' option when adding/removing items, and will default to true if
    'animateItems' is turned on.

    Tested with an in-progress change and animation turned on. Saw the items
    fade in when newly added and fade out when removed.

    Tested that existing configform lists weren't animated.

    Description From Last Updated

    We don't want to compare against true instead?

    daviddavid

    The default durations in query are a little long for my taste.

    daviddavid

    There's an extra blank line here.

    daviddavid

    Same question here.

    daviddavid

    The default durations in query are a little long for my taste.

    daviddavid
    reviewbot
    1. Tool: Pyflakes
      Ignored Files:
          djblets/static/djblets/js/configForms/views/listView.js
      
      
      
      Tool: PEP8 Style Checker
      Ignored Files:
          djblets/static/djblets/js/configForms/views/listView.js
      
      
    2. 
        
    david
    1. 
        
    2. Show all issues

      We don't want to compare against true instead?

      1. Nope. If animateItems is true, we assume animation for item changes, unless it's explicitly turned off. The only time we have any real control over options passed here is when we're resetting, since otherwise it's likely to be added from a new model creation in the collection, or when destroy() is called somewhere on the model (both of which are out of the caller's control, most of the time).

      2. That explanation doesn't seem to match your conditional. You only animate the item if both options.animate isn't false and this.animateItems is true.

        I might assign a local variable to the result of options && options.animate and then your conditional can check if (animate || this.animateItems)

      3. The options.animate !== false is assuming animation for items, unless explicitly turned off (options.animate = false, which the reset handler passes, since that's the only place we have real control over when adding items here). That matches my explanation, from what I can tell.

      4. The options.animate !== false says to animate if options.animate === true || options.animate === undefined.

      5. OK, I understand now. I think it would be a lot clearer if you assigned a local variable called something like overrideAnimate, and then did if (this.animateItems && overrideAnimate !== false)

      6. Okay. It's not actually overriding. It's just saying whether this particular item should be animated, if animation is turned on for this ListView. I'm not convinced a new variable really helps things any more than a comment would, but I'll pull it out and add a comment anyway.

    3. Show all issues

      The default durations in query are a little long for my taste.

      1. Having played with this a bit, it feels fine to me. It's a very quick transition in the list. I tried shortening the animation time, and it actually ends up looking glitchy, rather than animated, if it's any shorter, like it's just stuttering when showing the text. Maybe it has something to do with being primarily text-based, or something.

    4. Show all issues

      There's an extra blank line here.

    5. Show all issues

      Same question here.

    6. Show all issues

      The default durations in query are a little long for my taste.

    7. 
        
    chipx86
    reviewbot
    1. Tool: PEP8 Style Checker
      Ignored Files:
          djblets/static/djblets/js/configForms/views/listView.js
      
      
      
      Tool: Pyflakes
      Ignored Files:
          djblets/static/djblets/js/configForms/views/listView.js
      
      
    2. 
        
    david
    1. Ship It!

    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-0.8.x (0773fd0)
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (0773fd0)