-
Notifications
You must be signed in to change notification settings - Fork 516
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(searchbox): SearchBox should not change the state if SearchOnEnterKeyPress #2180
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,6 +37,10 @@ const KEY_SUPPRESS = 8; | |
* @return {Object} | ||
*/ | ||
|
||
const defaultQueryHook = function (query, setQueryAndSearch) { | ||
setQueryAndSearch(query); | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We use else where named functions, I think should try to enforce to use them when we do not need arrow functions: function defaultQueryHook(query, setQueryAndSearch) {
...
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like named functions too. Let's enforce that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like named functions too but not sure when they are assigned to a variable, does it bring any difference at runtime? It should not anymore. var superFn = function(){}
var superFn = function superFn(){} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you see the correct name in the call stack in the debugger :) |
||
const usage = `Usage: | ||
searchBox({ | ||
container, | ||
|
@@ -56,7 +60,7 @@ function searchBox({ | |
wrapInput = true, | ||
autofocus = 'auto', | ||
searchOnEnterKeyPressOnly = false, | ||
queryHook, | ||
queryHook = defaultQueryHook, | ||
}) { | ||
// the 'input' event is triggered when the input value changes | ||
// in any case: typing, copy pasting with mouse.. | ||
|
@@ -172,11 +176,6 @@ function searchBox({ | |
// Add all the needed attributes and listeners to the input | ||
this.addDefaultAttributesToInput(input, state.query); | ||
|
||
// always set the query every keystrokes when there's no queryHook | ||
if (!queryHook) { | ||
addListener(input, INPUT_EVENT, getInputValueAndCall(setQuery)); | ||
} | ||
|
||
// search on enter | ||
if (searchOnEnterKeyPressOnly) { | ||
addListener(input, 'keyup', ifKey(KEY_ENTER, getInputValueAndCall(maybeSearch))); | ||
|
@@ -186,18 +185,12 @@ function searchBox({ | |
// handle IE8 weirdness where BACKSPACE key will not trigger an input change.. | ||
// can be removed as soon as we remove support for it | ||
if (INPUT_EVENT === 'propertychange' || window.attachEvent) { | ||
addListener(input, 'keyup', ifKey(KEY_SUPPRESS, getInputValueAndCall(setQuery))); | ||
addListener(input, 'keyup', ifKey(KEY_SUPPRESS, getInputValueAndCall(maybeSearch))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading the comment saying that we will remove this when we drop IE8 support, isn't this already the case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a v1 update and we are still trying to be ie8 compatible on this version. |
||
} | ||
} | ||
|
||
function maybeSearch(query) { | ||
if (queryHook) { | ||
queryHook(query, setQueryAndSearch); | ||
return; | ||
} | ||
|
||
search(query); | ||
queryHook(query, setQueryAndSearch); | ||
} | ||
|
||
function setQuery(query) { | ||
|
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.
Should be based on callCount instead of callOnce to enforce that it's actually never called (and call count has the nice effect of comparing numbers)