methnen / m-chart

Manage data sets and display them as charts in WordPress.
Other
43 stars 22 forks source link

Fix custom Chart.js animation being overridden #171

Closed fcFn closed 1 year ago

fcFn commented 2 years ago

The current code overrides any custom animation settings for Chart.js charts defined in a theme or elsewhere. This fix merges the animation object with the onComplete property instead so that the custom animation is not removed.

methnen commented 2 years ago

@fcFn looks mostly good to me. I'd like to test this a bit from my end before merging it though and I repointed it at the 1.10 branch will be the next release.

methnen commented 1 year ago

@fcFn I'm merging this and will be pushing the change out with version 1.10 in a few minutes. But wanted to confirm what the goal is here.

Is the problem that the animation code where the render_done event is triggered is overwriting custom animations?

This sort of works around that by not applying the code that triggers the event unless there's no animation already in the chart settings?

Since some admin code relies on the event triggering this is potentially problematic but the problem is unlikely for most users so I'm gonna go ahead with it as is, but plan on reproaching it in a way where we can not overwrite animations like that but still always get the event trigger we need. I'm sure there's a way to do that but this release has been waiting for some time and I don't want to hold it up.

Thanks a ton for letting me know if I'm understanding the situation correctly.

fcFn commented 1 year ago

Hi, @methnen! Sorry for the delay in reply.

The details are quite fuzzy by now unfortunately. It should still trigger the event no matter what (as long as the animation actually completes, I suppose). If there is no custom animation, it will just add an animation property with only the onComplete inside (like it did in the previous revision). If there is, only the onComplete property will be overridden while the custom animation will remain.

Oh, and I just realized that ideally it shouldn't overwrite the custom onComplete too if there is one, probably by wrapping the custom onComplete call so it would both trigger the event and do whatever the user wanted, so my fix is a bit quick and dirty.

Thank you for merging the PR and for your work on the plugin!

methnen commented 1 year ago

Cool, thanks, I think I was understanding correctly and yeah I'll likely rewrite a bit in the next version to always preserve the onComplete. I'll have to look at how to best do that but at least I know I wasn't barking up the wrong tree. :)