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 chart event sync #123

Merged

Conversation

adelamarAtGalileo
Copy link

@adelamarAtGalileo adelamarAtGalileo commented Aug 16, 2023

Fixes Issue: #95

Context

This issue was caused by a change in github.com/chartjs/Chart.js/pull/9613 where the chart event controller started filtering the events to plugins by the event's native type:

Previously

const eventFilter = (plugin) => (plugin.options.events || this.options.events).includes(e.type);

Now

const eventFilter = (plugin) => (plugin.options.events || this.options.events).includes(e.native.type);

The plugin chartjs-plugin-crosshair syncs events by passing them through the chart's _eventHandler again, yet failed to specify the event's native.type

Fix

The fix was to implement a form of @bolau's suggestion (#95 (comment)) where we preserve the event.type and add the event.native.type

It seems that the fix by @geriatricdan is already contained in the repository code, but at the wrong place:

var newEvent = {
  type: e.original.type == "click" ? "mousemove" : e.original.type,  // do not transmit click events to prevent unwanted changing of synced charts. We do need to transmit a event to stop zooming on synced charts however.
  chart: chart,
  x: xScale.getPixelForValue(e.xValue),
  y: e.original.y,
  native: {
    // INSERTED HERE
    type: e.original.type == "click" ? "mousemove" : e.original.type,  // do not transmit click events to prevent unwanted changing of synced charts. We do need to transmit a event to stop zooming on synced charts however.
    buttons: buttons
  },
  stop: true
};

Testing

I've confirmed that this fix works on Chrome (Version 115.0.5790.170 (Official Build) (x86_64)) with:

"chart.js": "4.3.3",
"chartjs-plugin-crosshair": "2.0.0",
"react-chartjs-2": "5.2.0",

Example

Before After
broken-sync fixed-sync
Event does not sync Event does sync

@adelamarAtGalileo
Copy link
Author

@nategrift I see you made a change last week, any chance you could review this fix?

@nategrift nategrift changed the base branch from master to v4.0.1 August 16, 2023 23:57
@nategrift
Copy link
Collaborator

Hey @adelamarAtGalileo! Thanks for fixing this, seems to fix the issue. I am going to merge it into a new version branch v4.0.1 to bundle a bunch of improvements and fixes together. So you should see the issue fixed on npm soon.

Thanks for your contribution! ❤️

@nategrift nategrift merged commit 3472217 into AbelHeinsbroek:v4.0.1 Aug 17, 2023
1 of 5 checks passed
@NiTRoeSE
Copy link

NiTRoeSE commented Nov 24, 2023

Can confirm, this works with Chart.js 4+
The only thing i didn't get working is:

If charts have different heights than tooltip is not triggert in every position of the cursor.
Maybe anybody can explain or have a solution for this.

Bildschirmfoto 2023-11-24 um 12 27 18

Thanks in advanced !

@Admirated
Copy link

When we can see those fix on npm?

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

Successfully merging this pull request may close these issues.

4 participants