Skip to content

Start of Animation plugin#10574

Closed
benmccann wants to merge 1 commit into
chartjs:masterfrom
benmccann:animation-plugin
Closed

Start of Animation plugin#10574
benmccann wants to merge 1 commit into
chartjs:masterfrom
benmccann:animation-plugin

Conversation

@benmccann

@benmccann benmccann commented Aug 5, 2022

Copy link
Copy Markdown
Contributor

This stops some of the animation code (and dependencies such as helpers.easing) from being imported unless animations are being used. The animation plugin is somewhat minimal at the moment as a proof-of-concept, but there's a lot more we could move into the plugin. E.g. the next thing I'd like to move into the plugin are the animation options

Comment thread src/core/index.js
@@ -1,5 +1,5 @@
export {default as _adapters} from './core.adapters';
export {default as Animation} from './core.animation';
export {default as SingleAnimation} from './core.animation';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for the name change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin was also named Animation, so it was to avoid a clash. I thought about making the plugin something like AnimationPlugin, but none of the other plugins had a Plugin suffix. And I thought it'd be better to give the shorter name to the plugin because it's something users would use regularly whereas I think this class is only used in tests and unlikely to be referenced by users.

@stockiNail

Copy link
Copy Markdown
Contributor

Forgive my question but moving animation as plugin should all options related to animation be moved in plugins.animation node to be aligned with all other plugins?

@benmccann

Copy link
Copy Markdown
Contributor Author

Forgive my question but moving animation as plugin should all options related to animation be moved in plugins.animation node to be aligned with all other plugins?

Yes, that's exactly the plan. I just wanted to make sure people were generally okay with moving animation into a plugin. If so, I can make that change either in this PR or a follow up.

@kurkle

kurkle commented Aug 6, 2022

Copy link
Copy Markdown
Member

I'd be ok having the animations as a plugin/extension.

If its not using any hooks, maybe it should not be a plugin. But it could as well use the start/stop or install/uninstall hooks to attach to the Chart.

The controller default animations need a little thought, because I don't think we can have all the defaults in the animation plugin. (External chart types for example)

The tooltip is similar in that its defaults might not be needed for a controller to register. Maybe we can have the animation defaults importable and registrable separately for each controller.

@stockiNail

Copy link
Copy Markdown
Contributor

Yes, that's exactly the plan. I just wanted to make sure people were generally okay with moving animation into a plugin. If so, I can make that change either in this PR or a follow up.

Thank you @benmccann for feedback

The controller default animations need a little thought, because I don't think we can have all the defaults in the animation plugin. (External chart types for example)

@kurkle correct me if I'm wrong but also plugin default animations could be affected (i.e chart-plugin-annotation).

@kurkle

kurkle commented Aug 18, 2022

Copy link
Copy Markdown
Member

The controller default animations need a little thought, because I don't think we can have all the defaults in the animation plugin. (External chart types for example)

@kurkle correct me if I'm wrong but also plugin default animations could be affected (i.e chart-plugin-annotation).

True, good point @stockiNail

@kurkle

kurkle commented Aug 22, 2022

Copy link
Copy Markdown
Member

I guess this would need a bit more work to be efficient. Also I think this would fail miserably now with animations enabled in options and without the plugin registered.
The Animator should also be in the plugin (maybe the plugin should be called Animator to avoid SingleAnimation?)

@benmccann

Copy link
Copy Markdown
Contributor Author

yeah, this is going to take more thought. i'll go ahead and close it for now

@benmccann benmccann closed this Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants