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

proposal for #807, includes #550, SVG.off() multiple events and option argument #808

Merged
merged 6 commits into from
Feb 28, 2018

Conversation

Fuzzyma
Copy link
Member

@Fuzzyma Fuzzyma commented Feb 11, 2018

This PR is Work in Progress.

It moves the event handlers on the node as discussed in #807. It also changes #550 so that we now have fire() and dispatch() as two seperate methods.

Furthermore its now possible to remove multiple elements at once when calling SVG.off(). (Binding was already possible). It also adds the option argument to SVG.off() which is needed there.

I made this PR to see which changes are required to make the proposal work. Its not that much overhead to manage HTMLNodes.
Comments are welcome!

@Fuzzyma Fuzzyma requested review from wout and saivan February 11, 2018 12:16
@coveralls
Copy link

coveralls commented Feb 11, 2018

Coverage Status

Coverage decreased (-0.004%) to 98.125% when pulling df25328 on events-memory-leak into 2afc67f on 3.0.0.

Copy link
Member

@saivan saivan left a comment

Choose a reason for hiding this comment

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

It all looks good to me

expect(Object.keys(SVG.listeners[SVG.handlerMap.indexOf(rect3.node)]['event']['*']).length).toBe(2) // 2 listener on rect3

expect(SVG.listeners.length).toBe(listenerCnt + 3) // added listeners on 3 different elements
expect(Object.keys(rect.events['event']['*']).length).toBe(1) // 1 listener on rect
Copy link
Member

Choose a reason for hiding this comment

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

Whats this star doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The star is a wildcard namespace. So every event not bound to a particular namespace will get saved under the wildcard namespace

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay, sounds fair to me.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe though, it should be clearer? Maybe something like 'default'?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanted to take something which wouldnt be used by the user on accident

@saivan
Copy link
Member

saivan commented Feb 28, 2018

Go ahead and merge this and close its relevant issues if you're happy with it :)

@Fuzzyma Fuzzyma merged commit 3d1b02f into 3.0.0 Feb 28, 2018
@Fuzzyma Fuzzyma deleted the events-memory-leak branch November 8, 2018 13:22
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