-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Make event notification handler read the value from currentTarget, #5309
Make event notification handler read the value from currentTarget, #5309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. We do have to run this through Sauce though to see if we broke browser-compatibility (IE, looking at you). @azakus Do you mind pushing to a branch on our repo to kick off Travis for that?
test/unit/property-effects.html
Outdated
el.$.boundChild.customEventValue = 42; | ||
child.dispatchEvent(new Event('custom', {bubbles: true})); | ||
console.warn(el.$.boundChild.customEventValue); | ||
console.warn(el.$.boundChild.customEventValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, do you mind removing these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, fixed.
instead of target which may not be bound to any data. Fixes Polymer#5308
f1600f3
to
7a00b3c
Compare
I have pushed the branch to our repository. Travis build running at https://travis-ci.org/Polymer/polymer/builds/411197150?utm_source=github_status&utm_medium=notification |
Confirmed working 🎉 Merging this one. @tomalec Could you open a similar PR to master as well? |
Thanks! |
instead of the
target
which may not be bound to any data.Reference Issue
Fixes #5308
Related to 1.x issue #2497