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 detecting changed events #9050

Merged
merged 3 commits into from May 10, 2021
Merged

Conversation

joshkel
Copy link
Contributor

@joshkel joshkel commented May 7, 2021

Because this._listeners may contain both event handlers from options and internal event handlers for responsive support, the setsEqual check would often fail, causing event handlers to be unnecessarily detached and reattached and fired.

If I'm understanding correctly, this is the root cause of #9049.

Because `this._listeners` may contain both event handlers from options and internal event handlers for responsive support, the `setsEqual` check would often fail, causing event handlers to be unnecessarily detached and reattached and fired.

If I'm understanding correctly, this is the root cause of chartjs#9049.
Correctly update events when responsive property changes as well as when requested events change.
@kurkle
Copy link
Member

kurkle commented May 7, 2021

No complains from a quick look. Might take a couple of days before I can give a through review.

@kurkle kurkle requested a review from etimberg May 7, 2021 18:55
src/core/core.controller.js 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.controller.js Show resolved Hide resolved
src/core/core.controller.js Outdated Show resolved Hide resolved
@joshkel joshkel changed the title Fix detecting changed events (WIP) Fix detecting changed events May 10, 2021
@etimberg etimberg merged commit 1df4883 into chartjs:master May 10, 2021
@etimberg etimberg added this to the Version 3.3 milestone May 10, 2021
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