Isolate properties / modes from animation options #8332
Conversation
|
To me that's the opposite: I would still suggest
I'm not sure it's that confusing but at least it easier to manipulate them. Having I'm fine to leave |
| [[mode]](#animation-mode-configuration) | `object` | [defaults...](#default-modes) | Option overrides for update mode. Core modes: `'active'`, `'hide'`, `'reset'`, `'resize'`, `'show'`. See **Hide and show [mode]** example above. | ||
| [[property]](#animation-property-configuration) | `object` | `undefined` | Option overrides for a single element `[property]`. These have precedence over `[collection]`. See **Looping tension [property]** example above. | ||
| [[collection]](#animation-properties-collection-configuration) | `object` | [defaults...](#default-collections) | Option overrides for multiple properties, identified by `properties` array. | ||
| `modes` | `object` | [defaults...](#default-modes) | Option overrides for update modes. Core modes: `'active'`, `'hide'`, `'reset'`, `'resize'`, `'show'`. See **Hide and show [mode]** example above. |
simonbrunel
Jan 21, 2021
Member
I would suggest to rename this option to something clearer. For user not familiar with the update
argument, it's hard to get what is it and from where it comes.
What do you think of transitions
as suggested in this comment?
kurkle
Jan 21, 2021
Author
Collaborator
I think it might be better name for it. Just a lot of changes if doing mode
->transition
everywhere.
After a long discussion with @kurkle on slack, we came up with a few proposals. I detailed all of them in this Gist so it's easier to review. My preferences:
@etimberg @stockiNail @benmccann @emmcbd (and whoever else is interested): thoughts? |
@simonbrunel I had a look to your evaluation. Thanks a lot for the work done! Comparing with the master implementation, I have doubts about when Having a look to TS types, Furthermore, mainly based on my mindset, to have 3 (or 2) namespaces at same level of configuration which are referring to the same topic (animation) sounds not so readable. That said, my votes could be a bit different, but finally they didn't change the result. |
@simonbrunel @kurkle thanks for looking at a really detailed set of options. I can be on-board with option A in the gist. |
@stockiNail Thank you for your feedback.
Yes it is, however the fallback strategy requires to handle
Actually,
We could gather everything under // Proposal [a']: similar to [a] but one level deeper.
animation: {
default: { duration: 200 },
animations: { ... },
transitions: { ... }
}
// Proposal [b']: similar to [b]:
animation: {
duration: 200,
animations: {...},
transitions: {...}
} If we decide to go with |
@simonbrunel thank you very much for explanation. I think make more sense to go to scenario "a". |
I have the feeling that even if I like to have a namespace for topic, putting everything in Thanks again! |
I've never been super on-top of the animation stuff - I disable animations in my charts because they're not necessary for my use case where I want to maximize performance. So I don't have much of an opinion and have no objections to any change if you all agree. I won't dive into it too much unless you really need someone to be a tie breaker |
To be sure this does not get merged before updated to the A option
|
The documentation is still outdated, but other parts are good to review |
Reviewed everything except the docs |
rebased, had conflicts. |
Docs updated too |
5d5e48d
into
chartjs:master
Closes: #8213
I decided to propose the
properties
overanimations
and keep it nested underanimation
.options.animation.properties
looks more explicit to me thanoptions.animation.animations
.Having
options.animation
andoptions.animations
could be confusing.