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 support for change events on <select>
elements
#8943
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.
Thanks for the quick turnaround.
src/service/action-impl.js
Outdated
@@ -216,6 +216,9 @@ export class ActionService { | |||
}); | |||
event.detail = detail; | |||
} | |||
} else if (event.target.tagName.toLowerCase() === 'select') { |
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.
Recommend moving LHS to a local tagName
var and use a switch statement.
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.
good idea
src/service/action-impl.js
Outdated
detail[field] = String(value); | ||
} | ||
}); | ||
const tagName = event.target.tagName.toLowerCase(); |
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.
Nit: target
instead of event.target
.
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.
done
src/service/action-impl.js
Outdated
} | ||
break; | ||
case 'select': | ||
detail['value'] = target['value']; |
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.
detail.value = target.value;
?
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.
done
src/service/action-impl.js
Outdated
@@ -199,23 +199,30 @@ export class ActionService { | |||
addChangeDetails_(event) { | |||
const detail = {}; |
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.
const detail = map();
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.
done
src/service/action-impl.js
Outdated
detail[field] = String(value); | ||
} | ||
}); | ||
event.detail = detail; |
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.
Probably can take this out of the switch
to reduce dupe.
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.
ok, but we should only set this if detail
isn't empty
15e6d28
to
030316e
Compare
Mostly just expands on the work done to support
input
elements.Fixes #8939