-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix: listen for the enter key and trigger a change to emulate change … #11108
Conversation
js/foundation.slider.js
Outdated
this.inputs.off('keyup.zf.slider').on('keyup.zf.slider', function (e) { | ||
if(e.keyCode == 13) { | ||
var idx = _this.inputs.index($(this)); | ||
_this._handleEvent(e, _this.handles.eq(idx), $(this).val()); |
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 is duplicated with https://github.com/DanielRuf/foundation-sites/blob/edf6f79ff4c1ec20345990467ca3fe2e03b350ec/js/foundation.slider.js#L469
We could use a little helper on place like:
const handleChangeEvent = function(e) {
const idx = _this.inputs.index($(this));
_this._handleEvent(e, _this.handles.eq(idx), $(this).val());
};
this.inputs.off('keyup.zf.slider').on('keyup.zf.slider', function (e) {
if(e.keyCode == 13) handleChangeEvent.call(this, e);
});
this.inputs.off('change.zf.slider').on('change.zf.slider', handleChangeEvent);
What do you think ?
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, pushed a new commit. Tests are fine.
js/foundation.slider.js
Outdated
this.inputs.off('change.zf.slider').on('change.zf.slider', function(e) { | ||
var idx = _this.inputs.index($(this)); | ||
_this._handleEvent(e, _this.handles.eq(idx), $(this).val()); | ||
const handleChangeEvent = function(e) { |
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.
Not aligned ;)
(sorry)
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. That's why we need eslint with our own rule set.
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.
LGTM 👍. Thank you @DanielRuf
…nge-event-enter-11096 for v6.5.0 edf6f79 fix: listen for the enter key and trigger a change to emulate change event in IE 0804c90 fix: deduplicate the code for the event listeners aefb26b fix: properly indent lines Signed-off-by: Nicolas Coden <[email protected]>
IE (11) strictly follows the HTML specification and only triggers
change
when an input loses focus.Other browsers and Edge also trigger it when the enter key is pressed.
https://html.spec.whatwg.org/multipage/input.html#common-input-element-events
Also see
http://komlenic.com/231/internet-explorer-onchange-on-enter/
https://www.mediaevent.de/javascript/onchange.html
https://stackoverflow.com/questions/11415142/event-change-in-ie-not-working-when-pressing-enter-button
https://stackoverflow.com/questions/31802122/not-able-to-to-use-onchange-event-in-ie-11-using-jquery
Closes #11096