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: Closed (submitted)

Change Summary:

Pushed to release-0.8.x (0773fd0)
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (0773fd0)
Loading...