Skip to content
This repository has been archived by the owner on Apr 19, 2021. It is now read-only.

Add new features to custom tooltip #373

Merged

Conversation

2alin
Copy link
Contributor

@2alin 2alin commented Mar 30, 2019

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

Copy link
Contributor Author

@2alin 2alin left a 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.

src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
Copy link
Contributor

@klahnakoski klahnakoski left a 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.

src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
src/utils/chartJs/CustomTooltip.jsx Show resolved Hide resolved
@klahnakoski
Copy link
Contributor

@aimenbatool may you check to see if anything else is missed? I am unable to set you as a reviewer formally.

thank you!

@klahnakoski
Copy link
Contributor

@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.

@aimenbatool
Copy link
Contributor

aimenbatool commented Apr 1, 2019

@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.
Here is what is happening:

  • We can see data details on first time hovering
  • We click & lock data point
  • We now move the mouse and this time we don't see data details on mouse hover (which was happening earlier)
  • We can see data details upon clicking only after the first click

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 :)

@klahnakoski
Copy link
Contributor

@aimenbatool thank you for noticing that subtle behaviour. I added it as another issue.

@klahnakoski klahnakoski merged commit 93779a3 into mozilla-frontend-infra:master Apr 1, 2019
@2alin 2alin deleted the add-new-features-tooltip branch April 11, 2019 08:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

click-lock is not working on new tooltip Add arrow tip to tooltip Fix tooltip's wrong placement
3 participants