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

Event handling to use target instead currentTarget #5575

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

teroman
Copy link
Contributor

@teroman teroman commented Jun 19, 2018

If you attach event handlers to a container rather than directly to the canvas then the currentTarget is the container, event.target is the canvas that triggers the event.
It's useful to do this if you have many charts or are creating them dynamically.

If you attach event handlers to a container rather than directly to the canvas then the currentTarget is the container, event.target is the canvas that triggers the event.
It's useful to do this if you have many charts or are creating them dynamically.
@simonbrunel
Copy link
Member

I think this PR also fixes #5103, so maybe target is the way to go. @etimberg?

@teroman can you please share a jsfiddle that demonstrates that case?

@etimberg
Copy link
Member

In the case of the event handler attached to the canvas, currentTarget and target are the same, correct? I'm trying to think if this solves all the issues. In general, we want the canvas to be in it's own div so that resizing works, even when charts are dynamically generated.

@teroman
Copy link
Contributor Author

teroman commented Jul 12, 2018

Hi, sorry about the delay.

Here is a fiddle, https://jsfiddle.net/e8n4xd4z/11575/ The handler attached to the chart finds an element when it is clicked, the handler attached to the body does not.

@benmccann
Copy link
Contributor

@teroman what do you think about being able to add a unit test?

@teroman
Copy link
Contributor Author

teroman commented Aug 9, 2018

Hi,

I'm not sure how to write a test for an interactive event like this. I can't see any existing tests that call getElementAtEvent or getRelativePosition. If you could point me to any existing test for those I will try to adapt them to test this change.

@benmccann
Copy link
Contributor

I think all unit tests in interaction.test.js already check if these methods work correctly. Maybe that's enough. I don't know this section of the code well enough to understand the difference between target and currentTarget, so will leave this review to Simon and Evert

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

I agree, let's merge this change.

@simonbrunel simonbrunel merged commit 3830216 into chartjs:master Aug 10, 2018
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
If you attach event handlers to a container rather than directly to the canvas then the currentTarget is the container, event.target is the canvas that triggers the event. It's useful to do this if you have many charts or are creating them dynamically.
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.

4 participants