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

Fix onHover event not being triggered #4297

Merged
merged 2 commits into from May 28, 2017
Merged

Fix onHover event not being triggered #4297

merged 2 commits into from May 28, 2017

Conversation

ricardocosta
Copy link
Contributor

The core controller was looking at the wrong object (options.hover) to find the function to be called on hover. The function is provided on the top level options object (options.onHover).

Fixes #4296

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.

Per my comment in #4926 (at the exact same time this was opened) I think we need to update the docs not the code because otherwise this is a breaking change from v2.0.0

@etimberg
Copy link
Member

An alternative solution is to say that the docs are right and what we want but make the change in a way that keeps backwards compatibility with the old hover.onHover function. @simonbrunel thoughts?

@ricardocosta
Copy link
Contributor Author

@etimberg, @simonbrunel What do you think of exposing lastActive when invoking the onHover callback?
hoverCallback.call(me, e.native, me.active, me.lastActive);

This will allow developers interested only in knowing if the hovering state changed (active != lastActive) to return false on the callback and therefore "disregard" the mousemove events.

The core controller was looking at the wrong object (options.hover) to
find the function to be called on hover. The function is provided on the
top level options object (options.onHover).

Fixes #4296
@ricardocosta
Copy link
Contributor Author

@etimberg Per @simonbrunel's feedback on #4296, I've added the fallback to the "old" options.hover.onHover. 👍

// Need to call with native event here to not break backwards compatibility
hoverOptions.onHover.call(me, e.native, me.active);
hoverCallback.call(me, e.native, me.active);
Copy link
Member

Choose a reason for hiding this comment

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

Might be more concise using helpers.callback(options.onHover || options.hover.onHover, [e.native, me.active], me), then no need to test if hoverCallback is defined/null.

By using the helper function, there's no need to verify if the callback
is defined, as the helper already does that.
@etimberg etimberg merged commit dab0a7f into chartjs:master May 28, 2017
@simonbrunel simonbrunel added this to the Version 2.7 milestone May 30, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Fix onHover event not being triggered

The core controller was looking at the wrong object (options.hover) to
find the function to be called on hover. The function is provided on the
top level options object (options.onHover).

By using the helper function, there's no need to verify if the callback
is defined, as the helper already does that.

Fixes chartjs#4296
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.

None yet

3 participants