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

hover.animationDuration:0 doesn't disable animation tooltip with datalabels plugins #108

Closed
dutrieux opened this issue Jan 24, 2019 · 19 comments · Fixed by #119
Closed

hover.animationDuration:0 doesn't disable animation tooltip with datalabels plugins #108

dutrieux opened this issue Jan 24, 2019 · 19 comments · Fixed by #119
Assignees
Milestone

Comments

@dutrieux
Copy link

dutrieux commented Jan 24, 2019

When I use datalabels plugins with options.hover.animationDuration:0 (to disable animation with tooltip), the animation is not disable. If I remove the plugin Datalables (by removing chartjs-plugin-datalabels.js), the animation of tooltip is properly disable.

Test case : https://jsfiddle.net/dutrieux/w2vzeo3b/
Video for demo : https://youtu.be/Pd293lqSpK8

@simonbrunel simonbrunel added this to the Version 0.6 milestone Jan 29, 2019
@simonbrunel simonbrunel self-assigned this Jan 29, 2019
@rpearce
Copy link

rpearce commented Feb 12, 2019

Thank you for reporting this! I spent hours trying to track this down.

@etimberg
Copy link
Member

A possible workaround here is to set animation.duration to 0 if the initial animation isn't needed for your use case

@etimberg
Copy link
Member

Looks like this is fixed with the current master branch of the main chartjs repo. fiddle

It appears that chartjs/Chart.js#5331 was the solution. The previous code fallsback to animation.duration hence why the workaround I mentioned above works. I'm still not entirely sure why the plugin enables this behaviour though.

@rpearce
Copy link

rpearce commented Feb 12, 2019

A possible workaround here is to set animation.duration to 0 if the initial animation isn't needed for your use case

Thanks, but I unfortunately require the initial animations but need to alter the ones for hover, only :(


So is this not an issue with this plugin, specifically?

@simonbrunel
Copy link
Member

@etimberg I don't think it's fixed in the core repo, though not sure why your fiddle is working. For example, this one doesn't work while loading https://www.chartjs.org/dist/master/Chart.min.js.

@rpearce @dutrieux the issue doesn't really come from this plugin but the lack of a method to request a new draw from the Chart.js API. Currently, we call render() when we want to redraw labels. Unfortunately, when calling this method without arguments, a default duration is applied using animation.duration.

Explicitly calling this method with 0 doesn't solve the issue either because it will evaluate pending animations (e.g. tooltip) immediately, thus remove/break hover animation. The main problem is that currently, there is no way to animate things independently. That's something we need to enhance for v3.

@etimberg
Copy link
Member

That's really weird! The version I used was built against the latest master locally. We perhaps need the concept of layers that we can animate independently. Then the data labels plugin can redraw its layer as needed

@simonbrunel
Copy link
Member

We may want to create separate animation requests for each (animated) property, instead of a global animation that automatically interpolate all element properties base on a single easing value. And instead of global animation (or hover animation) options, we would define per property animation (or group of properties).

So for example, we could animate the background color of a point faster than its border color. Or one point position faster than another (using scriptable options). The animation framework would then be totally independent of the animated target, which is not the case currently.

@rpearce
Copy link

rpearce commented Feb 12, 2019

Thanks for explaining all of this

@nagix
Copy link

nagix commented Mar 9, 2019

Could we pass {lazy: true} to render option like eventHandler in Chart.js? This is for user interactions, so it might be a good idea to so do to enable more responsive interactions.

@simonbrunel
Copy link
Member

@nagix good point, I wouldn't have thought that lazy could fix that issue. Still testing a few more interaction cases and will submit a fix if that doesn't break anything else.

@simonbrunel
Copy link
Member

It seems to fix the following case:

animation.duration: 1000 && hover.animationDuration: 0

but the other way is still broken (no animation at all):

animation.duration: 0 && hover.animationDuration: 1000

The only workaround that seems to work is:

chart.render({duration: 1, lazy: true});

The 1 ms allows to defer the render to the next animation frame (i.e. animating), while lazy allows the event handler to override the current animation with the hover animation duration. If no duration given, it fallback to 0 (no animation) and elements will transition to latest values immediately.

Not sure if the 1ms will not introduce new issues?!

@nagix
Copy link

nagix commented Mar 9, 2019

No animarion with animation.duration is the expected behaviour, isn’t it?

@simonbrunel
Copy link
Member

It's not the case: https://jsfiddle.net/simonbrunel/yrn8jp2L/ (I removed the datalabels plugin), that's because hover animation duration overrides the global animation duration. It allows to disable initial animations while animating the tooltips and hover interactions.

@nagix
Copy link

nagix commented Mar 9, 2019

Does it make sense to use hover.animationDuration as the default value because hovering/tooltip animation and this event happen at the same time?

@simonbrunel
Copy link
Member

... because hovering/tooltip animation and this event happen at the same time

Not exactly true: if tooltip / hover are disable (or if there is no change), but the user have setup label interactions (e.g. change the color when hovering a label), this would trigger an animation for nothing (labels are not animated).

I don't think it makes sense for the plugin to use hover.animationDuration (actually, it doesn't work) or animation.duration because the plugin doesn't want an animation, it just need a single draw.

@nagix
Copy link

nagix commented Mar 11, 2019

Ok, I thought label properties are subject to animation. If the plugin doesn't want an animation, I think {duration: 1, lazy: true} is ok as the animation will be overridden or finish in the next frame.

By the way, I noticed that hover animation is not working at all, so opened chartjs/Chart.js#6129.

@simonbrunel
Copy link
Member

I need to test {duration: 1, lazy: true} a bit more because I think it breaks some use cases as well. The animation logic should really be revisited for v3 in order to allow each element (or whatever) to be animated independently. But until then, I think we need a new chart API that would:

  • do nothing if there is an animation in progress (i.e. draw() will be implicitly called)
  • else triggers a draw() call (easing would be 1 in this case)

Currently, chart.animating is useless because it can be false even if there is a running animation.

@simonbrunel
Copy link
Member

Version 0.6.0 has been released.

@rpearce
Copy link

rpearce commented Mar 14, 2019

Thanks for pushing this through

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants