-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
When update() is called, we need to reset the tooltip. #4840
Conversation
Looks good to me One suggestion perhaps not directly related to your PR would be to add a short doc for |
Should we have a unit test for this behavior? |
+1 for unit test |
@etimberg is this one safe enough for 2.7.1? |
I think this is safe enough for 2.7.1 but if we're hesitant we can wait until 2.8.0 |
Hmm. Travis errored again:
@etimberg can you rebase this against master to see if the attempted fix will solve the CI issue? |
@benmccann rebased! |
lgtm! |
@simonbrunel any thoughts on this one? |
How about #4991 with this update? I'm using 2.7.0 at the moment because I do not want my tooltips to hide on an update() call and I don't know how to manually prevent this behaviour or redraw them on the last active point. |
Any update on this? Is it out? |
Second that, any news on this? |
This PR has been merged and is in the latest version of Chart.js If you want the fix for #4991 please discuss it there |
@benmccann You mean 2.7.2? |
It's in 2.7.1 according to the release notes |
Hi. I'm running 2.7.3 (https://cdnjs.cloudflare.com/ajax/libs/Chart.js/2.7.3/Chart.min.js) and my custom tooltip still contains old data and is at the incorrect position after using "chart.update()". I add and remove data before calling chart.update();. I also tried 2.8.0 and it also does not work there. As you see in the image. The line updates correctly (shifts to the left). But the tooltip has not re-rendered. |
@basickarl That is correct, the tooltip is not updated in the current version of Chart.js. I'm working on a PR together with @kurkle to fix this. |
@Evertvdw Thanks for the quick reply man! I was under the impression that it was fixed with this merge. Thanks for clarifying dude! |
Resolves #3791. May also resolve #3165