Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(searchbox): SearchBox should not change the state if SearchOnEnterKeyPress #2180

Closed
wants to merge 2 commits into from
Closed

fix(searchbox): SearchBox should not change the state if SearchOnEnterKeyPress #2180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0536b74
fix(searchbox): SearchBox should not change the state if SearchOnEnte…
Jun 2, 2017
02c04ce
Merge branch 'develop' into fix/2105-bis
bobylito Jun 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/lib/url-sync.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ function getFullURL(relative) {
// IE <= 11 has no location.origin or buggy
function getLocationOrigin() {
// eslint-disable-next-line max-len
return `${window.location.protocol}//${window.location.hostname}${window.location.port ? `:${window.location.port}` : ''}`;
const port = window.location.port ? `:${window.location.port}` : '';
return `${window.location.protocol}//${window.location.hostname}${port}`;
}

// see InstantSearch.js file for urlSync options
Expand Down Expand Up @@ -126,7 +127,7 @@ class URLSync {
if (this.firstRender) {
this.firstRender = false;
this.onHistoryChange(this.onPopState.bind(this, helper));
helper.on('search', state => this.renderURLFromState(state));
helper.on('change', state => this.renderURLFromState(state));
}
}

Expand Down
12 changes: 8 additions & 4 deletions src/widgets/search-box/__tests__/search-box-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,8 @@ describe('searchBox()', () => {
simulateInputEvent('test', 'tes', widget, helper, state, container);
});

it('updates the query', () => {
expect(helper.setQuery.calledOnce).toBe(true);
it('does not update the query in the state', () => {
expect(helper.setQuery.calledOnce).toBe(false);
Copy link
Contributor Author

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)

});

it('does not search', () => {
Expand Down Expand Up @@ -477,10 +477,14 @@ describe('searchBox()', () => {
widget = searchBox({container, searchOnEnterKeyPressOnly: true});
});

it('performs the search on keyup if <ENTER>', () => {
it('does not perform search when typing', () => {
simulateInputEvent('test', 'tes', widget, helper, state, container);
expect(helper.search.callCount).toBe(0);
});

it('performs the search on keyup if <ENTER>', () => {
simulateKeyUpEvent({keyCode: 13}, widget, helper, state, container);
expect(helper.search.calledOnce).toBe(true);
expect(helper.search.callCount).toBe(1);
});

it('doesn\'t perform the search on keyup if not <ENTER>', () => {
Expand Down
19 changes: 6 additions & 13 deletions src/widgets/search-box/search-box.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ const KEY_SUPPRESS = 8;
* @return {Object}
*/

const defaultQueryHook = function (query, setQueryAndSearch) {
setQueryAndSearch(query);
};

Copy link
Contributor

Choose a reason for hiding this comment

The 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) {
 ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like named functions too. Let's enforce that!

Copy link
Contributor

Choose a reason for hiding this comment

The 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(){}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -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..
Expand Down Expand Up @@ -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)));
Expand All @@ -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)));
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) {
Expand Down