Consider Isolating mode, property, and collection animation options

[etimberg]

At present, the animation options can include settings for any of the given modes, along with user definable properties and collections. There runs a risk of collisions between the user defined attributes and the existing settings.

We should consider isolating these settings into sub keys.

Before

defaults.set('animation', {
	// Plain properties can be overridden in each object
	duration: 1000,
	easing: 'easeOutQuart',
	onProgress: noop,
	onComplete: noop,

	// Property sets
	colors: {
		type: 'color',
		properties: colors
	},
	numbers: {
		type: 'number',
		properties: numbers
	},

	// Update modes. These are overrides / additions to the above animations.
	active: {
		duration: 400
	},
	resize: {
		duration: 0
	},
	show: {
		colors: {
			type: 'color',
			properties: colors,
			from: 'transparent'
		},
		visible: {
			type: 'boolean',
			duration: 0 // show immediately
		},
	},
	hide: {
		colors: {
			type: 'color',
			properties: colors,
			to: 'transparent'
		},
		visible: {
			type: 'boolean',
			easing: 'easeInExpo' // for keeping the dataset visible almost all the way through the animation
		},
	}
});

After

defaults.set('animation', {
	// Plain properties can be overridden in each object
	duration: 1000,
	easing: 'easeOutQuart',
	onProgress: noop,
	onComplete: noop,

	// Collections
	collections: {
		
	},

	// Property sets
	properties: {
		colors: {
			type: 'color',
			properties: colors
		},
		numbers: {
			type: 'number',
			properties: numbers
		},
	},

	// Update modes. These are overrides / additions to the above animations.
	modes: {
		active: {
			duration: 400
		},
		resize: {
			duration: 0
		},
		show: {
			colors: {
				type: 'color',
				properties: colors,
				from: 'transparent'
			},
			visible: {
				type: 'boolean',
				duration: 0 // show immediately
			},
		},
		hide: {
			colors: {
				type: 'color',
				properties: colors,
				to: 'transparent'
			},
			visible: {
				type: 'boolean',
				easing: 'easeInExpo' // for keeping the dataset visible almost all the way through the animation
			},
		},
	}
});

[kurkle]

I'm not sure if it would be easier to understand, but its hard for me to say.

Also I'm not that worried about collisions. The set name is irrelevant, but those would be hard to maintain as an array.

[simonbrunel]

Also I'm not that worried about collisions.

Why I would not be allowed to call my collection active or onProgress? And what if we add a new option or mode under animation ?

We could gather property and collection (as they have the same shape) under a single property called animations since they are the definition of the animations. I would rename modes to transitions and move them also under a single property. Not sure I would keep everything under animation though, but move animations and transitions one level up.

{
  // Default animation options.
  animation: {
    duration: 1000,
    easing: 'easeLinear',
  },

  // Chart animations.
  animations: {
    color: {},   // property
    colors: {},  // collection
  },

  // Animations for chart events.
  transitions: {
    active: {
      color: {},   // property
      colors: {},  // collection
    },
    resize: {},
    show: {},
    hide: {},
  }
}

An alternative could be that since animations are animations for an update event, this could be an update transition.

Edit: that last alternative doesn't fully make sense because we can have animations not triggered by a transition.

[kurkle]

@simonbrunel in your example the properties and collections are still at same level. So you would not be able to call your collection backgroundColor for example.

We could change the "collection" to be a property with something like additionallyAppliesTo array of other property names that share the options defined there.

{
  animation: {
    duration: 1000,
    color: {
      type: 'color', 
      additionallyAppliesTo: ['backgroundColor'], 
   } 
  } 
} 

anyway, looks like we should at least separate the (base/default/root/inherited) animation options, target properties, property collections, and modes. transition might also be better name than mode

[stockiNail]

When I wrote the doc about animation I have to admit that wasn't so easy to understand how to configure it.
But that's probably depending on my lack of knowledge.
If I may, I think the proposal of @simonbrunel sounds better because it's clarifying the 3 different entities in specific nodes, apart the collision topic.
Just a doubt:
@simonbrunel why animations and transitions are not children of animation? Is there any reason that I missed?
I was thinking that being related to animation they could be children of it.

[simonbrunel]

@kurkle So you would not be able to call your collection backgroundColor for example.

In my original implementation, there was no "collection" vs "property". Everything is an animation that accepts a property input which can be either a string or an array. If undefined, it fallbacks to the animation key so for example:

animations: {
  color: {} ,        // shortcut for { property: 'color' }
  foo: {
    property: 'color',
  },
  colors: { 
    property: [
      backgroundColor,
      borderColor
    ]
  },
}

The key of this animations object is the animation id / name and this should not be in the same namespace as the common animation options or the modes/transitions.

@kurkle We could change the "collection" to be a property with something like additionallyAppliesTo

I'm not sure why we would need an extra property for this. I would probably drop that concept of collection in favor of what I described above, and rely on the property value being a string or an array.

@stockiNail Why animations and transitions are not children of animation

We could have all of them under animation indeed (i.e. animation: { animations, transitions, duration, ... }} but I find it easier to have them at the same level because you can describe (and understand) these options as:

type AnimationOptions = {
  duration: number;
  easing: string;
  ...
}
type Animations = {
  [key: string]: AnimationOptions & {
    from, to, type, fn,
  }
}
type Transitions = {
  [key: string]: Animations;
}

It may also be easier to manage / merge these options if not nested.

[simonbrunel]

Though, we should probably keep properties as name (instead of property) to make it clearer that it can accept more than one property. But maybe always expect an array of string. If undefined, if would fallback to properties: [animation_key].

[kurkle]

i think you described current functionality

[simonbrunel]

I know, but my whole point was that there is no difference between [property] and [collection], they are all animations and can be scoped under the same namespace (to answer your question). We should get rid of the collection concept in the docs since it's quite confusing and instead explain this fallback logic.