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

Issue 4991 #7084

Merged
merged 7 commits into from
Feb 25, 2020
Merged

Issue 4991 #7084

merged 7 commits into from
Feb 25, 2020

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Feb 10, 2020

On top of #7080
This is quite big, I'll split if needed.

Closes: #4991

Master
4991-master

pen for master

PR
4991-pr

pen for pr

src/controllers/controller.doughnut.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/controllers/controller.doughnut.js Outdated Show resolved Hide resolved
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.datasetController.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
@kurkle kurkle force-pushed the issue-4991 branch 3 times, most recently from 4bb92bf to bd5243e Compare February 11, 2020 07:42
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what happens if we declare only some of the types. That seems like it'd make it hard to use with DefinitelyTyped. I wonder if we want to hold off on that unless we can do them all. Maybe work on it in a separate branch if we want to go that way

src/controllers/controller.doughnut.js Outdated Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
src/core/core.element.js Outdated Show resolved Hide resolved
src/core/core.interaction.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member Author

kurkle commented Feb 11, 2020

I'm not sure what happens if we declare only some of the types. That seems like it'd make it hard to use with DefinitelyTyped. I wonder if we want to hold off on that unless we can do them all. Maybe work on it in a separate branch if we want to go that way

We would need to add the types to release, to be usable. (Also I don't think the DefinitelyTyped spec works at all with v3, so if we have a place for them here, I bet we get PR's for them)

@kurkle
Copy link
Member Author

kurkle commented Feb 11, 2020

removed the not needed changes.

etimberg
etimberg previously approved these changes Feb 12, 2020
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to this. Agree on the doc comments that @benmccann had

etimberg
etimberg previously approved these changes Feb 13, 2020
@benmccann
Copy link
Contributor

@kurkle this one will need to be rebased

etimberg
etimberg previously approved these changes Feb 14, 2020
const hoverOptions = options.hover;
// If the event is replayed from `update`, we should evaluate with the final positions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain more why the position from update might not be the final position?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Element's properties are its current state ("view" in 2.9) and the animations contain the target ("model" in 2.9). So when animations are enabled, after update and before first animation frame, elements x/y are exactly the same than before update. Only the animations were updated. The Animator then updates the elements in each frame.

etimberg
etimberg previously approved these changes Feb 17, 2020
@benmccann
Copy link
Contributor

Sorry for my delay on reviewing. Would you mind sharing an interactive example for this one? I want to play around and test a few things

@kurkle
Copy link
Member Author

kurkle commented Feb 19, 2020

@benmccann
Copy link
Contributor

In the codepen, if I leave my mouse over a segment of the pie and then don't touch it, the tooltip just stays in the same place and doesn't update the value it's displaying despite the data changing

@kurkle
Copy link
Member Author

kurkle commented Feb 19, 2020

In the codepen, if I leave my mouse over a segment of the pie and then don't touch it, the tooltip just stays in the same place and doesn't update the value it's displaying despite the data changing

Good catch! Fixed!

@benmccann
Copy link
Contributor

I found a couple more issues:

  • I left my mouse over Data 1 and it eventually popped over the border into Data 3
  • I left my mouse on Data 3 and for awhile it was moving to realign with the center of the pie piece with each animation, but then it just stopped moving and froze where it was

@benmccann
Copy link
Contributor

I saw you pushed a new commit. Now sure if you updated the codepen, but it's working the same for me

@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2020

I saw you pushed a new commit. Now sure if you updated the codepen, but it's working the same for me

I did update the pen. The code is in another pen though, so there could be caching.
Can you produce an animated gif of the issues, I can't seem to reproduce. I noticed a "pause" in moving when animations were finished when the update occurs (increasing the update interval to 2s made it obvious), and fixed that.

@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2020

Here's the same pen using master: https://codepen.io/kurkle/pen/MWwjvaw

@benmccann
Copy link
Contributor

It seems to be working pretty well now though I did get one quirk. I'm not quite sure how to make an animated gif, but here's a screenshot of an instance where the tooltip was placed a bit oddly. I think that it may happen when the mouse is in Data 1, that segment gets a data value of 0 (now the mouse is in Data 2 as a result), and then the next update when it reappears in Data 1 it does so in an unexpected location.

Screenshot from 2020-02-20 19-22-43

@kurkle
Copy link
Member Author

kurkle commented Feb 23, 2020

Additionally fixed the alignment, size and caret position of tooltip. Pen updated.

@kurkle kurkle requested a review from benmccann February 23, 2020 13:27
etimberg
etimberg previously approved these changes Feb 23, 2020
@benmccann
Copy link
Contributor

If I just leave my mouse stationary over one of the segments, the tooltip no longer moves as it should when the segment sizes change (I was looking at https://codepen.io/kurkle/pen/MWwjvaw)

@kurkle
Copy link
Member Author

kurkle commented Feb 23, 2020

If I just leave my mouse stationary over one of the segments, the tooltip no longer moves as it should when the segment sizes change (I was looking at https://codepen.io/kurkle/pen/MWwjvaw)

Look at the other pen, that one is how master works.

The one using this PR: https://codepen.io/kurkle/pen/yLNJwJZ

@benmccann
Copy link
Contributor

Sorry about that and thanks for the correction

It looks pretty good. I tried to slow it down quite a bit to do some final checks. I was following docs/configuration/animations.md and changed the settings to the below settings. When I move my mouse in between segments the tooltip moves pretty quickly even though I'd expect it to take 4s to move based on my settings. Am I doing something wrong? I'm wondering if maybe the docs need to be updated or if the tooltip animation speed isn't using the settings as it should

Chart.defaults.animation.duration = 10000;
Chart.defaults.animation.active.duration = 4000;
window.setInterval(RefreshData, 15000);

@kurkle
Copy link
Member Author

kurkle commented Feb 24, 2020

I filed a new issue about the animation documentation.
Here is a pen with the tooltip animation slowed down to 4000 too:
https://codepen.io/kurkle/pen/MWwJKxa

Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior in the pen all looks good to me. Just had a minor suggestion about adding some more explanation in the code. Thanks for the patience on this one

const hoverOptions = options.hover;
// If the event is replayed from `update`, we should evaluate with the final positions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand this comment a little? There's a lot going on here, so I just want to make sure I understand correctly.

I think the following is perhaps true:

  • It's referring the final position after this update
  • The event will have mouse coordinates from the last event. This should basically be where the mouse is currently

Why do we handle the event at the final position? Once the animation starts it will move it back to the initial position and then animate it back towards the final position?

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

Successfully merging this pull request may close these issues.

[BUG] Tooltip hides when update is called
3 participants