Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Isolate properties / modes from animation options #8332

Merged
merged 3 commits into from Feb 20, 2021
Merged

Conversation

@kurkle
Copy link
Collaborator

@kurkle kurkle commented Jan 20, 2021

Closes: #8213

I decided to propose the properties over animations and keep it nested under animation.

options.animation.properties looks more explicit to me than options.animation.animations.

Having options.animation and options.animations could be confusing.

@kurkle
Copy link
Collaborator Author

@kurkle kurkle commented Jan 20, 2021

Filename Size Change
dist/chart.esm.js 63 kB +59 B (0%)
dist/chart.js 78.9 kB +72 B (0%)
dist/chart.min.js 57.2 kB +22 B (0%)
Copy link
Member

@simonbrunel simonbrunel left a comment

options.animation.properties looks more explicit to me than options.animation.animations.

To me that's the opposite: properties is too generic and doesn't describe what it contains. It can be understood as the properties for animation (i.e. animation.properties.duration: 500). It's also a bit misleading because keys of this object are not necessarily properties (e.g. animation.properties.foobar.properties: 'color'). And having animation.properties[key].properties is a bit weird.

I would still suggest animations which describes better what is it. I don't find animation.animations an issue but if you don't like that name, then let's pick something else than properties.

Having options.animation and options.animations could be confusing.

I'm not sure it's that confusing but at least it easier to manipulate them. Having animations under animation requires to filter the animation object on merge. I guess if animation (or animations) was named differently, it would not be an issue to have them at the same level.

I'm fine to leave animations nested under animation if it's a general preference but I think it would provide a cleaner API if not nested. For example, we would never have scale.scales with scale containing default scale options and scales all scale items.

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

This comment has been minimized.

@simonbrunel

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?

This comment has been minimized.

@kurkle

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.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 23, 2021

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: a >> c > b >> d > master (reasons detailed in the Gist):

a b c d master
Consistency + + - + --
Robustness + + + + --
Readability ++ +/- + - --
Fallback logic ++ - + - --
Typing + +/- +/- +/- -
Verbosity +/- +/- + - ++
Breaking v2 + + + - +
Score 8 2 4 -2 -6

@etimberg @stockiNail @benmccann @emmcbd (and whoever else is interested): thoughts?

@stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Jan 23, 2021

@simonbrunel I had a look to your evaluation. Thanks a lot for the work done!

Comparing with the master implementation, animations is a concept which gatherers collection and properties concepts.
And that's good for me even if in my option these 2 concepts are not so clear to a normal user because it's not clear which properties you can use (apart the properties mentioned in the doc or in the samples). The risk is no user will really use it.

I have doubts about when animation and animations will be used. Maybe I'm wrong but the animation will be triggered by a mode (or transition) therefore I had the feeling that those 2 namespaces sounds like "defaults" for transitions.

Having a look to TS types, Animations is a map of Animation instances. Therefore, also Animation can (or must) define all properties that currently we have in the collections and properties (like keys), for the fallback. And therefore my doubt is if Animations is really needed due to you can configure the mode (which is already an extension of Animation) as you want. Sounds a redundant configuration level. But maybe I'm wrong.

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.
Maybe if the animations namespace will be changed (or removed), the evaluation could change.

@etimberg
Copy link
Member

@etimberg etimberg commented Jan 23, 2021

@simonbrunel @kurkle thanks for looking at a really detailed set of options. I can be on-board with option A in the gist.

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Jan 23, 2021

@stockiNail Thank you for your feedback.

... those 2 namespaces sounds like "defaults" for transitions

Yes it is, however the fallback strategy requires to handle animation  and animations separately, which is easier if not nested. animation contain defaults for all animations, while animations represents the actual animation "items" (similar to scales). If animations: {} is empty, the chart is not animated.

Having a look to TS types, Animations is a map of Animation instances.

Actually, Animations is a bit more than a map of Animation because it also contain properties (e.g. animations.foo.properties: ['color']) - I will fix my Gist. So no, animation doesn't contain properties / collections, these are replaced by animations.

...to have 3 (or 2) namespaces at same level ... referring to the same topic (animation) sounds not so readable.

We could gather everything under animation:

// 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 [b] then maybe it's better to also have transitions under animation (i.e. [b']). Though, it makes the fallback logic even more complicated and I'm not sure it really helps to understand the options hierarchy. If we go with [a] then I would not nested everything under a unique namespace (i.e. [a']).

@stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Jan 23, 2021

@simonbrunel thank you very much for explanation.

I think make more sense to go to scenario "a".

@stockiNail
Copy link
Contributor

@stockiNail stockiNail commented Jan 23, 2021

We could gather everything under animation:

I have the feeling that even if I like to have a namespace for topic, putting everything in animation could be too verbose.
With animations namespace, the scenario "a" looks like anyway better than the others for all things that you reported.

Thanks again!

@benmccann
Copy link
Collaborator

@benmccann benmccann commented Jan 24, 2021

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

@etimberg etimberg added this to the Version 3.0 milestone Jan 30, 2021
@kurkle kurkle dismissed etimberg’s stale review Feb 1, 2021

To be sure this does not get merged before updated to the A option

@kurkle kurkle force-pushed the kurkle:isolate branch from 9bf047e to b069274 Feb 19, 2021
@kurkle
Copy link
Collaborator Author

@kurkle kurkle commented Feb 19, 2021

Filename Size Change
dist/chart.esm.js 65.1 kB +96 B (0%)
dist/chart.js 82.5 kB +106 B (0%)
dist/chart.min.js 58.8 kB +66 B (0%)
dist/chunks/helpers.segment.js 18.4 kB +18 B (0%)
@kurkle
Copy link
Collaborator Author

@kurkle kurkle commented Feb 19, 2021

The documentation is still outdated, but other parts are good to review

Copy link
Member

@etimberg etimberg left a comment

Reviewed everything except the docs

src/core/core.config.js Show resolved Hide resolved
src/core/core.scale.js Show resolved Hide resolved
kurkle added 2 commits Feb 19, 2021
@kurkle kurkle force-pushed the kurkle:isolate branch from 73485a6 to 4af5990 Feb 19, 2021
@kurkle
Copy link
Collaborator Author

@kurkle kurkle commented Feb 19, 2021

rebased, had conflicts.

@kurkle
Copy link
Collaborator Author

@kurkle kurkle commented Feb 19, 2021

Docs updated too

@etimberg etimberg merged commit 5d5e48d into chartjs:master Feb 20, 2021
9 of 10 checks passed
9 of 10 checks passed
build (ubuntu-latest)
Details
build
Details
build (windows-latest)
Details
finish
Details
codeclimate 1 issue to fix
Details
Coveralls - ubuntu-latest-chrome First build on isolate at 95.275%
Details
Coveralls - ubuntu-latest-firefox First build on isolate at 95.186%
Details
Coveralls - windows-latest-chrome First build on isolate at 95.29%
Details
Coveralls - windows-latest-firefox First build on isolate at 95.171%
Details
coverage/coveralls First build on isolate at 95.29%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants