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

use addEventListener to allow { handleEvent } #104

Closed
wants to merge 2 commits into from

Conversation

serapath
Copy link

@serapath serapath commented Dec 6, 2017

to "fix" #103

I can see the unconvenience, that this PR removes the ability to remove the listener later.

If this is unwanted, maybe val can be checked and addEventListener would only be used if val is an object and not a function. Maybe in case of object, it could be assigned to el.on[event.type] too, so later it's possible to call el.removeEventListener('click', el.onclick) which a user would need to do before overwriting el.onclick with a custom handler.

@bcomnes
Copy link
Collaborator

bcomnes commented Dec 6, 2017

How does addEventListener differ from setting the handler as an attribute? Does the eventListener get removed when the dom node is discarded like it is with attribute handlers?

still add it to `el[p] = val` so a user later has the chance to `el.removeEventListener(event.type, el['on' + event.type])`
@serapath
Copy link
Author

serapath commented Dec 6, 2017

For some reason, it seems, the { handleEvent } thing only works when addEventListener is used, but if an object is assigned like el.onclick = { handleEvent }, it doesn't do anything.

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 19, 2019

Do these get garbage collected the same way attribute assignments work? The only issue I see is if these have a different GC characteristic.

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 19, 2019

Otherwise, I think supporting this would be better.

FWIW, in the meantime, I've been using fast-on-load hooks to add event listeners this way if needed.

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 19, 2019

Oh wait, these questions were addressed, and yes this does seem to work.

@bcomnes
Copy link
Collaborator

bcomnes commented Aug 19, 2019

Going to close this since its stale. Open to a discussion but for now that seems best.

Thoughts: we support attributes in nanohtml. Trying to pretend addEventListener is an attribute listener is adding complexity. You should manage node related listeners in your on-load events, so that they aren't firing when the node is out of the page to begin with, which is the perfect place to add these types of listeners.

@bcomnes bcomnes closed this Aug 19, 2019
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.

None yet

2 participants