-
Notifications
You must be signed in to change notification settings - Fork 386
Conversation
This is definitely something that we can skip...
With this implementation all the connectors can propagate the information that the search is currently stalled (meaning that it seems that there are slow network issues). Currently it is only implemented in the searchbox.
Deploy preview ready! Built with commit c9e156c |
@bobylito since you updated the yarn version under package.json you'll need to update also the travis script :) |
Sure will do. 👍 |
stories/SearchBox.stories.js
Outdated
<WrapWithHits searchBox={false} linkedStoryGroup="SearchBox"> | ||
<SearchBox showLoading={true} /> | ||
</WrapWithHits> | ||
)) | ||
.add('playground', () => ( | ||
<WrapWithHits searchBox={false} linkedStoryGroup="SearchBox"> |
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.
Could you add the showLoading props as knobs on playground story?
Oh yes I will do that 👌
|
That's a very nice feature here :) Adding this loading info to other connector can be done quickly! If I'm not mistaken there's only the spinner missing here. Do you need action/help of someone here @bobylito? |
No I'm good for the implementation. I'm not quite settled on the implementation details for all InstantSearch. Once I'm sure of the consistency then I'll finish the feature :) I'm wondering how to properly test that though... 🤔 |
We should totally add this to autoComplete connector. |
For consistency with InstantSearch.js
It was a mistake to try to fix the current behaviour of this flag. We should keep it as legacy and remove it during the next major.
And fix tests
I think that now it should be good to go :) cc @samouss |
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.
Nice! 👍 I left few comments.
.travis.yml
Outdated
@@ -9,7 +9,7 @@ addons: | |||
before_install: | |||
- export DISPLAY=:99.0 | |||
- sh -e /etc/init.d/xvfb start | |||
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.2.0 | |||
- curl -o- -L https://yarnpkg.com/install.sh | bash -s -- --version 1.2.1 |
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.
The version should match the one in the package.json
=> 1.3.2
@@ -172,6 +176,10 @@ class SearchBox extends Component { | |||
</svg> | |||
); | |||
|
|||
const loadingIndicatorComponent = this.props.loadingIndicatorComponent ? ( | |||
this.props.loadingIndicatorComponent | |||
) : <DefaultLoadingIndicator />; |
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.
We could provide a default props for loadingIndicatorComponent
with the value DefaultLoadingIndicator
. It will avoid the need to do the condition.
role="search" | ||
{...cx( | ||
'wrapper', | ||
this.props.showLoadingIndicator && |
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.
We can probably create a variable for this.props.showLoadingIndicator && this.props.isSearchStalled
since we are using this expression three times. It will be a bit less verbose in the code.
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.
And do you have an idea for a name for this variable?
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.
Nevermind, it's gonna be isSearchStalled :)
props.stalledSearchDelay === undefined || | ||
props.stalledSearchDelay === null | ||
? 200 | ||
: props.stalledSearchDelay; |
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.
We could provide a default props for stalledSearchDelay
with the value 200
. It will avoid the need to do the condition.
}) { | ||
const baseSP = new SearchParameters({ | ||
...searchParameters, | ||
index: indexName, | ||
...highlightTags, | ||
}); | ||
|
||
let stalledSearchTimer = null; | ||
let isSearchStalled = false; |
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.
You could store the variable isSearchStalled
directly into the store. Then we can avoid to keep track of the same value twice.
@@ -25,6 +26,9 @@ class SearchBox extends Component { | |||
onReset: PropTypes.func, | |||
onChange: PropTypes.func, | |||
|
|||
isSearchStalled: PropTypes.bool, |
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.
We could set this value required since isSearchStalled
will be always pass to the component.
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.
From a usage perspective, I would argue that this value is only useful if showLoadingIndicator
is true and the only value that we really about is true
. Considering that, do you think it should still be marked as required?
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.
We can leave it as it it then, but when we define an optional props it's recommended to always give them a default value. So we can put a default value to false
on the isSearchStalled
. WDYT?
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 WFM 👍
@@ -134,6 +134,7 @@ class List extends Component { | |||
selectItem(items[0], this.resetQuery); | |||
} | |||
}} | |||
showLoadingIndicator={false} |
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.
We could provide a default props for showLoadingIndicator
with the value false
. It will avoid the need to force the value to false
when we don't want the indicator to display.
…/react-instantsearch into feat/searchbox-loading-indicator
I took care of your comments. Feel free to have another look ;) |
@@ -227,12 +227,14 @@ export default function createConnector(connectorDesc) { | |||
metadata, | |||
resultsFacetValues, | |||
searchingForFacetValues, | |||
isSearchStalled, | |||
} = store.getState(); | |||
const searchState = { |
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.
We can take advantage of your changes for rename the variable to searchResults
. The third parameter pass to getProvidedProps
is searchResults
. The second one is searchState
. So it's bit confusing to have searchResults
called searchState
🙂
The search is now considered stalled before first results.
…/react-instantsearch into feat/searchbox-loading-indicator
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.
Sorry but I miss some things during the previous review. We could add some test cases for the SearchBox
component.
> | ||
{loadingIndicatorComponent} | ||
</div> | ||
)} |
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.
We could add a test for this logic. Verify depending on the given props that the loading component is displayed or not (with a Snapshot).
); | ||
const submitComponent = this.props.submitComponent; | ||
const resetComponent = this.props.resetComponent; | ||
const loadingIndicatorComponent = this.props.loadingIndicatorComponent; |
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.
We could use destructuring assignment for those variables.
…/react-instantsearch into feat/searchbox-loading-indicator
Happy to oblige :) Here you are :) |
loadingIndicatorComponent, | ||
resetComponent, | ||
submitComponent, | ||
} = this.props; |
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.
We could merge the destructuring props with the one above. Because we destruct the same attributes two times (see line 203
).
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.
Nice feature @bobylito! 👍 🎉
Thanks!
<a name="4.3.0"></a> * reset page with multi index ([#665](#665)) ([865b7dc](865b7dc)) * **SearchBox:** use hidden over style to hide loader ([#714](#714)) ([ed5eb77](ed5eb77)) * test recipes ([#740](#740)) ([de2cc37](de2cc37)) * track all index in the manager ([#660](#660)) ([793502b](793502b)) * loading indicator ([#544](#544)) ([189659e](189659e)) * **Highlight:** support array of strings ([#715](#715)) ([8e93c6a](8e93c6a))
This PR adds a loading indicator based on the proposal made here. However this version has changed a little:
<InstantSearch>
and<SearchBox>
. The first one is for configuring the timer and the other is for displaying a loading indicator.This is a POC implementation and the there is no design for the indicator (it's a red background) on purpose. Happy to discuss the API and the implementation.