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.

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

Loading indicator #2315

Closed
bobylito opened this issue Sep 7, 2017 · 10 comments
Closed

Loading indicator #2315

bobylito opened this issue Sep 7, 2017 · 10 comments

Comments

@bobylito
Copy link
Contributor

bobylito commented Sep 7, 2017

When the search has latency it is pretty hard to understand why the results don't update. It's especially true when the search happens on mobile (where there are a lot of latency even on good network). One way to tell the user that something is happening is to display a spinner.

The good thing is that the JS Helper now implements a flag and an event to notify that the search has been completed.

What I propose is a standalone loading connector and an implementation in the searchbox (so that we can replace the magnifying glass with a spinner)

@Haroenv
Copy link
Contributor

Haroenv commented Sep 7, 2017

We should definitely have a reasonable delay (like 200ms) before showing the spinner.

@bobylito
Copy link
Contributor Author

bobylito commented Oct 13, 2017

I have done a PoC implementation in the playlistFinder. What I ended up doing is binding the search and searchQueueEmpty. What I propose for the integration of this in our searchbox is the following:

widget

Add a parameter showLoading: {boolean|{delay: number = 200}} = false

connector

Add two hooks for when there is a search and when the search queue is empty. Since we want to implement the best practice by default, the search hook will implement the debounce. I don't know if the other hook should be any smarter.

onStalledSearch: (function) => undefined
onSearchQueueEmpty: (function) => undefined

@samouss
Copy link
Contributor

samouss commented Oct 13, 2017

Looks like a good solution! 👍

Just for the widget parameters part, I think it's easier to handle / configure with two options rather than one:

  • one for enable or not the loader
  • one for configure or not the loader

For the user the name of the parameter could indicate that it can only pass a boolean attribute, but it can be an object too. For the implementation it's easier to reason about a parameter who have only one type than many types :)

WDYT ?

@mthuret
Copy link
Contributor

mthuret commented Oct 13, 2017

I'm wondering how this could be extended to be used in other context. For instance, you might not want to have this logic on the search box but on a button when modifying a refinement list (common pattern on mobile for instance).

Should we add this showLoading options on every widget/connectors? Can it be an autonomous widget?

@bobylito
Copy link
Contributor Author

For the user the name of the parameter could indicate that it can only pass a boolean attribute, but it can be an object too. For the implementation it's easier to reason about a parameter who have only one type than many types :)

This kind of API is instantsearch.js signature style. The problem with standalone parameters is that you can end up with inconsistent set of options. When grouped liked that you can use the easy one with the default by using just true or go deep and fill in the object.

Should we add this showLoading options on every widget/connectors? Can it be an autonomous widget?

I don't think it makes sense to have it everywhere because I don't see the point in a refinement list for example. However I see a use case on hits for fading the results until the search is complete (PoC to try, out of Q4 scope).

The autonomous widget is a great idea. We were just discussing it with @ronanlevesque . Glad you came up with it. However it should not be hard to implement it with the connectSearchBox connector proposed (and not care about the refine)

@bobylito
Copy link
Contributor Author

I'm gonna move on with this proposal unless you have something to add @mthuret @samouss? (cc @iam4x @Haroenv @vvo @ronanlevesque)

@samouss
Copy link
Contributor

samouss commented Oct 17, 2017

Works for me 👍

@mthuret
Copy link
Contributor

mthuret commented Oct 17, 2017

I don't think it makes sense to have it everywhere because I don't see the point in a refinement list for example.

I think on mobile it can happen in more spaces than just the searchbox and the hits. I mentioned the stats widget earlier, that's because your have screens that do not necessarily include the searchbox or the hits. However I must admit that's very mobile oriented.

I'm good to start with the SearchBox though.

Also a note for later: on react-instantsearch they'll probably something to look after regarding the connectStateResults connector (that today contain the loading boolean info).

@bobylito
Copy link
Contributor Author

Ok thanks @mthuret I'll be careful

@bobylito
Copy link
Contributor Author

bobylito commented Feb 8, 2018

Done ✅

@bobylito bobylito closed this as completed Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants