feat(core): provide information about stalled search to widgets #2569

bobylito · 2017-11-09T18:57:27Z

This PR is an alternative implementation of #2509. This is currently a PoC.

At the core of this new implementation is a change when the render part of the lifecycle of the widget. With this implementation, the render can be called when the instantsearch detects that a search is stalled. This means that they will be called more often. However, this is not likely to have a big impact on the performance as it should be the responsability of the widgets to render only when necessary.

On the implementation of the connector and the widget, the work is trivial. It is merely a question of providing the isSearchStalled flag to the rendering. The logic is factored in instantsearch, which means that the usage in other widgets will be as easy.

Having done this implementation, I think it's the way to go. WDYT? Happy to answer any questions.

PS: this new implementation has been inspired by what can be done in react-instantsearch and vue-instantsearch. If we were to implement them using this library, this means that we need to fit their logic.

PPS: this can be tested here: https://deploy-preview-2569--algolia-instantsearch.netlify.com/v2/dev-novel/?selectedStory=SearchBox.default

This commit adds a new step that can update the UI. In this change, the widgets are rendered when there are new results (previous behaviour) and when the search is found to be stalled. The information is propagated through a new option `searchMetadata` to the render method of the widgets. This is an object that contains the flag `isStalledSearch` that is set to `true` when there are still pending search after a delay (for now 200ms).

algobot · 2017-11-09T19:00:49Z

Deploy preview ready!

Built with commit 737d28e

https://deploy-preview-2569--algolia-instantsearch.netlify.com

samouss

Nice 👍

I clearly prefer this implementation since it's not tightly coupled to the SearchBox widget 🎉

vvo · 2017-11-10T13:16:11Z

I like the implementation, as discussed earlier in a meeting, some questions still:

However, this is not likely to have a big impact on the performance
Can you try a small manual perf test in the browser to see if there's any obvious issue/diff with this implementation?

It should be the responsability of the widgets to render only when necessary.

Can you provide an example of a widget rendering that would avoid re-rendering when the widget do not care about the isSearchStalled flag? Just curious here how to do it.

Finally, for reference, can you provide a small concrete example of what's happening under the hood in a scenario where this is implemented? Something like:

user types 'a'
flag becomes true
widget render once..

Something like that. So that we all understand fully what's happening.

Haroenv · 2017-11-10T15:55:04Z

This looks really nice, I think we should go this route. I guess we can add some componentShouldUpate hooks?

bobylito · 2017-11-10T16:08:30Z

Can you provide an example of a widget rendering that would avoid re-rendering when the widget do not care about the isSearchStalled flag? Just curious here how to do it.

That's a very speculative on my side here I guess. I can try to implement it on a widget / connector to see what happens.

Something like that. So that we all understand fully what's happening.

Ok I'll do a schema.

This looks really nice, I think we should go this route. I guess we can add some componentShouldUpate hooks?

I don't know if that's necessary but that can be done, indeed.

bobylito · 2017-11-16T15:47:37Z

I will move on with this solution and close the other. Thanks for the feedbacks :)

bobylito · 2017-11-16T16:48:45Z

…tsearch.js into poc/central-loading-state

bobylito · 2017-11-24T16:48:06Z

Hello everyone,

This is the final version. Here is the API:

InstantSearch constructor / factory has a new option: stalledSearchDelay which configures the time before the stalled search kicks in.
Also InstantSearch provides a new searchMetadata attribute with the options sent to the widgets at render. It's not provided at init because there is no search yet (therefore no metadata about search). The searchMetadata is an object with a single key isSearchStalled.

We have a new set of options for the loading indicator on the search box:

{
  /* ... other searchbox options */
  loadingIndicator: true,
  // or
  loadingIndicator: {
    template,
    cssClasses: {root: ''}
  } 
}

The connector provides a new information to the searchbox rendering: isSearchStalled, if the search is stalled.

Haroenv · 2017-11-24T17:57:52Z

CI failed cause one commit message was too long (merge 1d0aa21) so you might need to rebase

bobylito · 2017-11-24T18:59:39Z

Thanks @Haroenv :D

Merge commits are automatically generated and are already standardized by git, therefore there is no point in checking their length. Worst truncating them would make use loose information.

samouss

Good job 👍

I just left two comments on documentation.

samouss · 2017-11-27T10:02:09Z

src/widgets/search-box/search-box.js

+/**
+ * @typedef {Object} SearchBoxLoadingIndicatorOption
+ * @property {function|string} template Template used for displaying the button. Can accept a function or a Hogan string.
+ * @property {{root: string}} [cssClasses] CSS classes added to the reset buton.


Should be the loading indicator root element.

samouss · 2017-11-27T10:03:14Z

src/widgets/search-box/search-box.js

@@ -173,8 +207,9 @@ searchBox({
 * @property  {string|HTMLElement} container CSS Selector or HTMLElement to insert the widget.
 * @property  {string} [placeholder] Input's placeholder.
 * @property  {boolean|SearchBoxPoweredByOption} [poweredBy=false] Define if a "powered by Algolia" link should be added near the input.
- * @property  {boolean|SearchBoxResetOption} [reset=true] Define if a reset button should be added in the input when there is a query.
+ * @property  {boolean|SearchBoxResetOption} [reset=false] Define if a reset button should be added in the input when there is a query.


Is it an intended change? The default value is still true in the code.

vvo · 2017-11-27T10:04:51Z

src/connectors/search-box/connectSearchBox.js

@@ -35,6 +35,8 @@ Full documentation available at https://community.algolia.com/instantsearch.js/c
 * @property {function(string)} refine Sets a new query and searches.
 * @property {function()} clear Remove the query and perform search.
 * @property {Object} widgetParams All original `CustomSearchBoxWidgetOptions` forwarded to the `renderFn`.
+ * @property {boolean} isSearchStalled `true` if the search results takes more than a certain time to come back


Should we document "a certain time"? I would be curious as a dev to know more. This may be too vague

indeed there is a default value. Will doc that.

Done (only happen a few lines after :))

vvo · 2017-11-27T10:14:58Z

src/widgets/search-box/search-box.js

+
+  if (loadingIndicator) {
+    if (isSearchStalled) {
+      containerNode.firstChild.classList.add('stalled-search');


Should this CSS class name follow ais- conventions? And be documented somewhere?

Oh yes sure. Also it doesn't work if you use an input as container :/

Maybe document that part, but I won't be surprised as a dev for it not to be supported.

vvo · 2017-11-27T10:15:52Z

src/widgets/search-box/search-box.js

@@ -173,8 +207,9 @@ searchBox({
 * @property  {string|HTMLElement} container CSS Selector or HTMLElement to insert the widget.
 * @property  {string} [placeholder] Input's placeholder.
 * @property  {boolean|SearchBoxPoweredByOption} [poweredBy=false] Define if a "powered by Algolia" link should be added near the input.
- * @property  {boolean|SearchBoxResetOption} [reset=true] Define if a reset button should be added in the input when there is a query.
+ * @property  {boolean|SearchBoxResetOption} [reset=false] Define if a reset button should be added in the input when there is a query.


Is it false by default now to have a reset button?

Will double check 👍

vvo · 2017-11-27T10:16:28Z

src/widgets/search-box/search-box.js

 * @property  {boolean|SearchBoxMagnifierOption} [magnifier=true] Define if a magnifier should be added at beginning of the input to indicate a search input.
+ * @property  {boolean|SearchBoxLoadingIndicatorOption} [loadingIndicator=false] Define if a loading indicator should be added at beginning of the input to indicate that search is currently stalled.


Should we make it true by default, since it's a new feature and might bring good behaviour by default?

I was wondering. The idea behind false by default is that we don't change any existing behavior / style, especially if the user doesn't use the provided stylesheets.

Yep that's the safest, if we were to do a new breaking version we would make it true by default, cc @mthuret can we add that to the feedback/product loop?

Happy to break in v3 ;)

agreed, false for now, breaking to true in v3 👍

bobylito · 2017-11-27T13:22:16Z

The behavior and loading indicator have been validated by @sebnvzt :D

iam4x

Love it, very nice work @bobylito 👍

<a name=2.3.0></a> # [2.3.0](v2.3.0-beta.7...v2.3.0) (2017-11-30) ### Bug Fixes * **InstantSearch.dispose:** dont call of URLSync widget ([#2604](#2604)) ([3234b12](3234b12)) ### Features * **core:** provide information about stalled search to widgets ([#2569](#2569)) ([d104be1](d104be1))

Alex S added 2 commits November 9, 2017 19:37

feat(searchbox): implements a loading indicator

a3a5947

bobylito requested review from vvo, iam4x, samouss and mthuret November 9, 2017 18:57

chore(lint): fix prettier and eslint check

b95c3b0

samouss reviewed Nov 10, 2017

View reviewed changes

Merge branch 'develop' into poc/central-loading-state

0e37c0c

bobylito mentioned this pull request Nov 16, 2017

feat(searchbox): provide a loading indicator #2509

Closed

vvo and others added 9 commits November 16, 2017 17:51

Merge branch 'develop' into poc/central-loading-state

75d7cf2

feat(core): add option to configure stalled search delay

59fdbaf

Merge branch 'poc/central-loading-state' of github.com:algolia/instan…

1d0aa21

…tsearch.js into poc/central-loading-state

feat(searchbox): add options to set a loading indicator

6821e09

Merge branch 'develop' into poc/central-loading-state

4ecf773

chore: update test to match the lib changes

a4d4b89

chore(search-box-test): remove sinon

ca4ed04

chore(connectSearchBox): remove sinon from test

852e4ab

chore(doc): add isStalledSearch on connectSearchBox rendering options

d5398f5

Alex S added 2 commits November 24, 2017 20:02

chore(validation): merge commit length should not be checked

7a450b1

Merge commits are automatically generated and are already standardized by git, therefore there is no point in checking their length. Worst truncating them would make use loose information.

test(core): test new render step with stalled search

ea2d14f

vvo added 2 commits November 27, 2017 10:19

Merge branch 'develop' into poc/central-loading-state

16475ab

Merge branch 'develop' into poc/central-loading-state

0cd59a5

samouss reviewed Nov 27, 2017

View reviewed changes

vvo suggested changes Nov 27, 2017

View reviewed changes

Alex S added 4 commits November 27, 2017 11:49

chore(searchbox): test with input tag container

e3a9940

chore(doc): fix inconsistencies and add details

dc5ee62

chore(searchbox): use .ais-stalled-search and fix use of input

5583d18

chore(doc): fix content

7e9157e

vvo approved these changes Nov 27, 2017

View reviewed changes

iam4x approved these changes Nov 27, 2017

View reviewed changes

bobylito changed the base branch from develop to feat/2.3 November 27, 2017 17:39

bobylito and others added 2 commits November 27, 2017 18:42

Merge branch 'feat/2.3' into poc/central-loading-state

36c5dcd

chore: merge with 2.3, update snapshot

737d28e

bobylito merged commit d104be1 into feat/2.3 Nov 28, 2017

bobylito deleted the poc/central-loading-state branch November 28, 2017 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): provide information about stalled search to widgets #2569

feat(core): provide information about stalled search to widgets #2569

bobylito commented Nov 9, 2017 •

edited

Loading

algobot commented Nov 9, 2017 •

edited

Loading

samouss left a comment

vvo commented Nov 10, 2017

Haroenv commented Nov 10, 2017

bobylito commented Nov 10, 2017

bobylito commented Nov 16, 2017 •

edited

Loading

bobylito commented Nov 16, 2017

bobylito commented Nov 24, 2017 •

edited

Loading

Haroenv commented Nov 24, 2017

bobylito commented Nov 24, 2017

samouss left a comment

samouss Nov 27, 2017

samouss Nov 27, 2017

vvo Nov 27, 2017

bobylito Nov 27, 2017

bobylito Nov 27, 2017

vvo Nov 27, 2017

bobylito Nov 27, 2017

vvo Nov 27, 2017

vvo Nov 27, 2017

bobylito Nov 27, 2017

vvo Nov 27, 2017

bobylito Nov 27, 2017

vvo Nov 27, 2017

bobylito Nov 27, 2017

Haroenv Nov 27, 2017

bobylito commented Nov 27, 2017

iam4x left a comment

		* @property {boolean\|SearchBoxMagnifierOption} [magnifier=true] Define if a magnifier should be added at beginning of the input to indicate a search input.
		* @property {boolean\|SearchBoxLoadingIndicatorOption} [loadingIndicator=false] Define if a loading indicator should be added at beginning of the input to indicate that search is currently stalled.

feat(core): provide information about stalled search to widgets #2569

feat(core): provide information about stalled search to widgets #2569

Conversation

bobylito commented Nov 9, 2017 • edited Loading

algobot commented Nov 9, 2017 • edited Loading

samouss left a comment

Choose a reason for hiding this comment

vvo commented Nov 10, 2017

Haroenv commented Nov 10, 2017

bobylito commented Nov 10, 2017

bobylito commented Nov 16, 2017 • edited Loading

bobylito commented Nov 16, 2017

bobylito commented Nov 24, 2017 • edited Loading

Haroenv commented Nov 24, 2017

bobylito commented Nov 24, 2017

samouss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobylito commented Nov 27, 2017

iam4x left a comment

Choose a reason for hiding this comment

bobylito commented Nov 9, 2017 •

edited

Loading

algobot commented Nov 9, 2017 •

edited

Loading

bobylito commented Nov 16, 2017 •

edited

Loading

bobylito commented Nov 24, 2017 •

edited

Loading