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

add possibility to bind events on custom elements instead of the whole svg #153

Closed
wants to merge 5 commits into from
Closed

Conversation

santoror
Copy link

Added eventBindSelector

@santoror santoror closed this Oct 10, 2015
@santoror santoror changed the title Master add possibility to bind events on custom elements instead of the whole svg Oct 10, 2015
@santoror santoror reopened this Oct 10, 2015
@ariutta
Copy link
Collaborator

ariutta commented Oct 11, 2015

Hi @venomis,

Thanks for the pull request! @bumbu is currently most active in managing this project, so his preference is most important in choosing how to merge this. I just had a few comments.

  1. @bumbu prefers to rely on automatic semicolon insertion, so let's stick with his preference here. (Is that still true @bumbu? I usually add the semicolons myself, but we'll stick with your preference here).
  2. We want to limit the core of this project to a reasonable size, so in issue Allow for adding plugins #98, @bumbu and I have been thinking about how we could restructure it to allow for plugins while keeping the core small. Do you have thoughts on a plugin model, and could this pull request be a plugin?
  3. Why can't code add a single event listener to the parent element and then filter the event stream by `event.target' instead of adding multiple event listeners directly to the targets, which can hurt performance?

@bumbu
Copy link
Owner

bumbu commented Oct 16, 2015

Hi @venomis,

Thanks for the pull request. I saw your messages on stackoverflow and this pull request makes sense. But I have just one comment: please remove semicolons that you added in this pull request. It just makes the pull request messy, so it is hard to understand what actually was added/edited.

@ariutta - I agree with you that it would look better as a plugin, but it seems that it will require quite a lot of refactor as from what I know main problem that is solved by this pull request is that plugin prevents events bubbling. This pull request solves the problem partially by listening and preventing events only from specific elements.
And about performance - it should make no difference as here only one selector (and not multiple) is used.

@santoror
Copy link
Author

Hi guys ( @ariutta , @bumbu ),
i've removed all the commas included into the request but the diff still look messy.
Let me know if it's ok or i need to revert all

@bumbu bumbu closed this in 479fb29 Nov 12, 2015
@bumbu
Copy link
Owner

bumbu commented Nov 12, 2015

Hi @venomis,

Sorry that it took so much time. I did add this functionality to the library (but with small changes).
I also made it work with zooming (not only pan events) and added events removal. Here's the documentation section about it.

Thank you for your work!

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.

3 participants