feat: provide a loading indicator #354

bobylito · 2017-11-03T13:25:38Z

This PR provides a way for the searchbox to display a spinner when the search is stalled.

The approach taken here is similar to the one from react-instantsearch: the mechanism that detects the stalled search is actually implemented at the top level. As in RIS this makes the implementation of this state update easier in multiple widgets, which seems like a good strategy.

On the UI implementation, because vue-IS doesn't implement any style, the magnifying glass svg is toggle with a spinner in the template. The effect is good as far as I can tell.

Missing elements:

actual API to customize the parameters
tests

Question:

not sure about the implementation in store.js/waitUntilSync @rayrutjes

Related to algolia/instantsearch#2509

Haroenv

yarn lint:fix

Haroenv · 2017-11-03T13:44:51Z

src/components/SearchBox.vue

@@ -3,13 +3,29 @@
    <slot>
      <ais-input :search-store="searchStore" :placeholder="placeholder" :autofocus="autofocus"></ais-input>
      <button type="submit" :class="bem('submit')">
-        <svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 40 40">
+        <svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 40 40" v-if="!searchStore.isSearchStalled">


Works really nice 😄

Maybe have this as an option though

Haroenv · 2017-11-03T13:45:15Z

src/components/SearchBox.vue

          <title>{{ submitTitle }}</title>
          <path
            d="M26.804 29.01c-2.832 2.34-6.465 3.746-10.426 3.746C7.333 32.756 0 25.424 0 16.378 0 7.333 7.333 0 16.378 0c9.046 0 16.378 7.333 16.378 16.378 0 3.96-1.406 7.594-3.746 10.426l10.534 10.534c.607.607.61 1.59-.004 2.202-.61.61-1.597.61-2.202.004L26.804 29.01zm-10.426.627c7.323 0 13.26-5.936 13.26-13.26 0-7.32-5.937-13.257-13.26-13.257C9.056 3.12 3.12 9.056 3.12 16.378c0 7.323 5.936 13.26 13.258 13.26z"
            fillRule="evenodd"
          />
        </svg>
+        <svg width="1em" height="1em" viewBox="0 0 38 38" xmlns="http://www.w3.org/2000/svg" stroke="#000" v-if="searchStore.isSearchStalled">


this should be a slot I think

Haroenv

no more blockers now I think

Haroenv · 2017-11-03T14:33:57Z

src/components/SearchBox.vue

          <title>{{ submitTitle }}</title>
          <path
            d="M26.804 29.01c-2.832 2.34-6.465 3.746-10.426 3.746C7.333 32.756 0 25.424 0 16.378 0 7.333 7.333 0 16.378 0c9.046 0 16.378 7.333 16.378 16.378 0 3.96-1.406 7.594-3.746 10.426l10.534 10.534c.607.607.61 1.59-.004 2.202-.61.61-1.597.61-2.202.004L26.804 29.01zm-10.426.627c7.323 0 13.26-5.936 13.26-13.26 0-7.32-5.937-13.257-13.26-13.257C9.056 3.12 3.12 9.056 3.12 16.378c0 7.323 5.936 13.26 13.258 13.26z"
            fillRule="evenodd"
          />
        </svg>
+        <svg width="1em" height="1em" viewBox="0 0 38 38" xmlns="http://www.w3.org/2000/svg" stroke="#000" v-if="showLoadingIndicator && searchStore.isSearchStalled">


Would still be nice to have this as a slot :)

Indeed. Will make this happen :)

Haroenv · 2017-11-03T14:38:44Z

src/store.js

+};
+
+const onHelperSearch = function() {
+  if (!this._stalledSearchTimer) {


not sure if this is a big deal, but maybe it could not be done only if the stalled search feature is enabled? Might be hard to find out about it here...

The feature is always there, for simplicity sake.

bobylito · 2017-12-08T15:17:05Z

I've updated this PR to be consistent with the other InstantSearch implementations.

it's good

rayrutjes

Super nice @bobylito .
I made a few comments.

For me the only real thing I would change is remove the new options parameter introduced on all factory methods.

The reasoning being that the state should only originate from a single source of truth.

I hope that makes sense.

rayrutjes · 2017-12-11T14:42:26Z

src/components/SearchBox.vue

@@ -24,8 +44,7 @@
  </form>
 </template>

-<script>
-import algoliaComponent from '../component';
+<script>import algoliaComponent from '../component';


Looks like the weird syntax is coming back here @Haroenv . Expected?

not expected, maybe @bobylito's vim setup, should be put on the new line IMO

I'll double check. Maybe I didn't update the packages that do the formating 🤔

So it's not an error from any linter / formatter. I might have just removed it by mistake :/

I'm pretty sure it's because of the linter or something.
Happens to me all the time but can't remember what the real issue was.

rayrutjes · 2017-12-11T14:46:55Z

src/components/Index.vue

@@ -72,7 +76,8 @@ export default {
    if (!this.searchStore) {
      this._localSearchStore = createFromAlgoliaCredentials(
        this.appId,
-        this.apiKey
+        this.apiKey,
+        { stalledSearchDelay: this.stalledSearchDelay }


Can we consider not adding another "options" parameter, and introduce a getter instead?
So far, the Store is just proxying as much as possible.
Also, if tomorrow the stalledSearchDelay is added to the helper, we won't require the option anymore and it would be an issue to be able to have it in the helper + as an option, because we won't know which one to keep.

I see what you mean. However my thinking behind choosing this as an option is that this is a feature in the scope of InstantSearch, not the helper. This might evolve but the way it is implemented right now is the way it is in the other libraries.

rayrutjes · 2017-12-11T14:48:39Z

src/components/SearchBox.vue

@@ -2,7 +2,27 @@
  <form role="search" action="" @submit.prevent="onFormSubmit">
    <slot>
      <ais-input :search-store="searchStore" :placeholder="placeholder" :autofocus="autofocus"></ais-input>
-      <button type="submit" :class="bem('submit')">
+      <div v-if="showLoadingIndicator" :style="searchStore.isSearchStalled ? 'display:block;' : 'display:none;'" :class="bem('loading-indicator')" >


I would probably go for v-show="showLoadingIndicator && searchStore.isSearchStalled" and remove the style attribute.

Agreed for that it would make more sense given the vue-specifics. However I chose that in order to have consistency accross all implementations. This is the exact same markup and changes on it depending on the value of isSearchStalled in all implementations.

rayrutjes

All good 🎸

Haroenv · 2017-12-12T10:16:11Z

src/components/SearchBox.vue

@@ -2,7 +2,27 @@
  <form role="search" action="" @submit.prevent="onFormSubmit">
    <slot>
      <ais-input :search-store="searchStore" :placeholder="placeholder" :autofocus="autofocus"></ais-input>
-      <button type="submit" :class="bem('submit')">
+      <div v-if="showLoadingIndicator" :style="searchStore.isSearchStalled ? 'display:block;' : 'display:none;'" :class="bem('loading-indicator')" >


You should use the hidden attribute, which implies display: none; but is better for a11y

:hidden="!searchStore.isSearchStalled"

bobylito · 2017-12-12T15:24:58Z

Updated per your suggestion @Haroenv thanks :)

…ading-indicator feat: provide a loading indicator

feat(loading-indicator): initial implementation

6bdf9a7

bobylito added the WIP DO NOT MERGE label Nov 3, 2017

bobylito self-assigned this Nov 3, 2017

bobylito requested a review from rayrutjes November 3, 2017 13:25

bobylito changed the title ~~feat(searchbox): provide a loading indicator~~ feat: provide a loading indicator Nov 3, 2017

Haroenv previously requested changes Nov 3, 2017

View reviewed changes

Haroenv reviewed Nov 3, 2017

View reviewed changes

bobylito added 4 commits November 3, 2017 15:07

chore(searchbox): rm unused computed prop

b11c125

feat(loading-indicator): add API

148577f

chore(loading-indicator): add story

56442bd

chore: lint fix

06419ca

Haroenv reviewed Nov 3, 2017

View reviewed changes

bobylito added 4 commits December 4, 2017 17:31

Merge branch 'master' into feat/loading-indicator

4c6d112

chore: align with other implementations + tests

da6f4d7

chore(doc): update documentation for Index

328cb84

chore: rename stalledSearchTimeout into stalledSearchDelay

25a07f6

bobylito removed the WIP DO NOT MERGE label Dec 8, 2017

rayrutjes reviewed Dec 11, 2017

View reviewed changes

chore: syntax

75e39fd

rayrutjes approved these changes Dec 11, 2017

View reviewed changes

Haroenv reviewed Dec 12, 2017

View reviewed changes

chore(SearchBox): use hidden attribute to hide/show elements

e2e50bf

chore: update snapshot

07e706c

bobylito merged commit 192ecee into master Jan 29, 2018

bobylito deleted the feat/loading-indicator branch January 29, 2018 16:51

Haroenv pushed a commit to algolia/instantsearch that referenced this pull request Dec 28, 2022

Merge pull request algolia/vue-instantsearch#354 from algolia/feat/lo…

fdbf93f

…ading-indicator feat: provide a loading indicator

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: provide a loading indicator #354

feat: provide a loading indicator #354

bobylito commented Nov 3, 2017 •

edited

Loading

Haroenv left a comment

Haroenv Nov 3, 2017

Haroenv Nov 3, 2017

Haroenv left a comment

Haroenv Nov 3, 2017

bobylito Dec 5, 2017

Haroenv Nov 3, 2017

bobylito Dec 8, 2017

bobylito commented Dec 8, 2017

rayrutjes left a comment

rayrutjes Dec 11, 2017

Haroenv Dec 11, 2017

bobylito Dec 11, 2017

bobylito Dec 11, 2017

rayrutjes Dec 11, 2017

rayrutjes Dec 11, 2017

bobylito Dec 11, 2017

rayrutjes Dec 11, 2017

bobylito Dec 11, 2017

rayrutjes left a comment

Haroenv Dec 12, 2017

Haroenv Dec 12, 2017

bobylito commented Dec 12, 2017

feat: provide a loading indicator #354

feat: provide a loading indicator #354

Conversation

bobylito commented Nov 3, 2017 • edited Loading

Haroenv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haroenv 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

bobylito commented Dec 8, 2017

rayrutjes 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

rayrutjes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobylito commented Dec 12, 2017

bobylito commented Nov 3, 2017 •

edited

Loading