-
Notifications
You must be signed in to change notification settings - Fork 180
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
Update numeric slider values on updateSearch #1111
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.
Thank you very much!
I changed the approach to use a custom event Would you have another look @yihui? In particular I'm wondering if the custom event name should be namespaced somehow. |
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.
I thought for a while, and perhaps we can pass an extra parameter to the input
event handler to decide whether to throttle the search. You can find jQuery's trigger()
documentation here: https://api.jquery.com/trigger/ The idea is that our handler fun
takes two arguments e
(event) and immediate
(whether to search immediately or throttle the search). immediate
is false
by default, and we set it to true
in the updateSearch()
method, e.g.,
$(td).find('input').first().val(v).trigger('input', [true]);
I feel that might be a little simpler than defining an updatesearch
event for all types of inputs. Let me know if the idea is not clear to you. I can also tweak the PR by myself. Thanks!
Yeah that makes sense. I can pivot to that approach. |
Ready for a review again @yihui. |
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.
Looks perfect now! I'll make a cosmetic change and merge soon. Thank you so much!
Fixes #1110.