-
Notifications
You must be signed in to change notification settings - Fork 68
Add new features to custom tooltip #373
Add new features to custom tooltip #373
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klahnakoski I've commented some stuff worth to point out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, and it is clearly solves the immediate problem. I will add @aimenbatool as a reviewer because she had the sharp eyes to catch what I missed last time.
@aimenbatool may you check to see if anything else is missed? I am unable to set you as a reviewer formally. thank you! |
@2alin We will wait for @aimenbatool to chime in. If you have any other PRs, be sure to base them off of this branch, which should be merged by Monday morning. |
@2alin @klahnakoski Great work. This looks perfect. I will take some time out and definitely read the code. 👏👏👏 A quick thing: When we click one data point and lock it then we can't see any other data point on mouse hover.
Drawback: We will have to click an unknown data point and lock it to see details. This is the behavior I'm talking about. Deploy #242 and check. @armenzg can tell better if it is good to add it back or not. Rest is an awesome work @2alin :) |
@aimenbatool thank you for noticing that subtle behaviour. I added it as another issue. |
This fixes #370, fixes #371, fixes #372 and continues #361.
@klahnakoski
It looks quite amazing, I think you will feel as proud as I feel right now. 😁
Tooltip on action (12s): https://youtu.be/2j-5VvuG08Q
To consider: sometimes the tooltip arrow looks like it's in the wrong position (a little) compared to the data point, but that's chartjs fault. Just for testing, I rendered the custom and default tooltips at the same time, and both arrow tips match (almost) perfectly: (39s) https://youtu.be/Vn5xAwOEHWs (recorded before the locking feature).
More comments in the code.
ping: @armenzg @aimenbatool