Issue #54 -- Create a 'loading results' indicator.

[reficul31]

This is in reference to the issue. I have added the animation so that the maintainers can see the animation and then comment what is to be done. Please inform me if I have violated any styling convention. I will change it at once.

[Treora]

Sorry, I forgot the most important comment: nice animation! :+1:

[reficul31]

I have a question. How about we remove the epic all together and just move the change of state of isFetching to the setQuery reducer? Something like this:

function setQuery(state, {query}) {
    return {...state, query, isFetching:true}
}

function setSearchResult(state, {searchResult}) {
    return {...state, searchResult, isFetching:False}
}

This will eliminate the use for mergeMaps.

[Treora]

How about we remove the epic all together and just move the change of state of isFetching to the setQuery reducer?

Would be simpler indeed, but fetching only starts after debouncing the query. Counting parallel searches would not work then either. So it seems better to have a separate reducer for the state of running searches.

[reficul31]

So do you feel we require 2 reducers? 1st to change the isFetching to 1 2nd to change the isFething to 0 And how about renaming the component Icon to Loader?

[Treora]

I was thinking setSearchResults can still set isFetching to zero. But if we do not show the animation during automatic updates of the results (on db changes), and if we count the number of running searches, we should keep better track of things.

How about not doing epics, doing those two reducers as you say (but increment/decrementing), and refreshSearchResults gets an extra parameter showLoadingIndicator, which if true will run those reducers before and after the search, respectively.

On 2017-03-08 17:36, Shivang Bharadwaj wrote:

So do you feel we require 2 reducers? 1st to change the isFetching to 1 2nd to change the isFething to 0

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/WebMemex/webmemex-extension/pull/57#issuecomment-285093137, or mute the thread https://github.com/notifications/unsubscribe-auth/AD_0sUXS93O2WqAH36xnh0PKjN3ZckyWks5rjtkogaJpZM4MVBUp.

[reficul31]

@Treora I am sorry I am a bit confused.

and refreshSearchResults gets an extra parameter showLoadingIndicator, which if true will run those reducers before and after the search, respectively.

By refreshSearchResults do you mean refreshSearchResultsUponQueryChange? What is the purpose of the showLoadingIndicator? From what I understood:

const refreshSearchResultsUponQueryChange = action$ => action$
    .ofType(actions.setQuery.getType())
    .debounceTime(500) // wait until typing stops for 500ms
    .mergeMap(() => {
                actions.incrementLoader()
                actions.refreshSearch()
                actions.decrementLoader()
}))

Do you mean something like this? I still can't understand the meaning of showLoadingIndicator and the need to pass it as a parameter.

[Treora]

By refreshSearchResults do you mean refreshSearchResultsUponQueryChange?

I meant the action. Does it make more sense then? :)

[reficul31]

Yes thanks for the clarification. It makes more sense now. But I still don't understand the use of the showLoadingIndicator. Also I tried it out this code.

export function refreshSearch() {
    return function (dispatch, getState) {
        dispatch(incLoader())       
        const query = ourState(getState()).query
        const oldResult = ourState(getState()).searchResult
        filterVisitsByQuery({query}).then(searchResult => {
            // First check if the query and result changed in the meantime.
            if (ourState(getState()).query !== query
                && ourState(getState()).searchResult !== oldResult) {
                // The query already changed while we were searching, and the
                // currently displayed result may already be more recent than
                // ours. So we did all that effort for nothing.
                return
            }
            // Set the result to have it displayed to the user.
            dispatch(setSearchResult({searchResult}))
        })
            dispatch(decLoader())
    }
}

The state was changed too fast. The loader didn't even appear. So instead I have done incLoader increments the state to +1 and setResults does a -1. Then the loader works fine. I have removed the decLoader() reducer altogether.

To summarize:

1. Added a new reducer incLoader(). Which changes isFetching to +1.
2. Called dispatch(incLoader()) in the refreshSearchResults action.
3. Added a new state change in setSearchResults, where isFetching is set to -1.
4. Inverted the view logic.
5. Renamed the component to Loader.
6. Indented the CSS.
7. Added newline.

[Treora]

The state was changed too fast. The loader didn't even appear.

The dispatch(decLoader()) should of course be put in the callback function (the then part), otherwise it executes directly. I suggest to read into Promises and asynchronicity if you are not familiar with this kind of pattern. Also adding the showLoadingIndicator argument that I mentioned, you'd get something like this:

export function refreshSearch({showLoadingIndicator}) {
    return function (dispatch, getState) {
        if (showLoadingIndicator) {
            dispatch(incLoader())
        }
        const query = ourState(getState()).query
        const oldResult = ourState(getState()).searchResult
        filterVisitsByQuery({query}).then(searchResult => {
            if (showLoadingIndicator) {
                dispatch(decLoader())
            }
            // First check if the query and result changed in the meantime.
            if (ourState(getState()).query !== query
                && ourState(getState()).searchResult !== oldResult) {
                // The query already changed while we were searching, and the
                // currently displayed result may already be more recent than
                // ours. So we did all that effort for nothing.
                return
            }
            // Set the result to have it displayed to the user.
            dispatch(setSearchResult({searchResult}))
        })
    }
}

I would use more self-explanatory and consistent function & variable names throughout the code by the way. Source code is primarily written for humans, not computers. :)

[reficul31]

Thanks a lot @Treora for being so clear and helpful. I adjusted the code according to the above snippet given and also gave the functions more self explanatory names. Sorry for being so lost but I just wanted to know when should the loader run. Currently it is running:

1. Initially
2. Whenever the query is executed
3. Whenever db change takes place Thanks for the review

[Treora]

Thanks a lot @Treora https://github.com/Treora for being so clear and helpful. I adjusted the code according to the above snippet given and also gave the functions more self explanatory names. Sorry for being so lost but I just wanted to know when should the loader run. Currently it is running:

1. Initially
2. Whenever the query is executed
3. Whenever db change takes place Thanks for the review

Yes those three I think. I thought only the first and second would pass showLoadingIndicator: true, hence the need for adding it as an argument.

[reficul31]

Sorry for the delay. This new PR features.

1. Made 2 reducers(incrementLoadCounter, decrementLoadCounter)
2. Passing a variable to the refreshSearch function to load the Loader
3. Change the call of the function wherever necessary.
4. The loader runs on just 2 occasions. First initially when all the queries are fetched from the db. Second when a query change takes place.

Please tell me if further changes are required. They shall be implement immediately.

[reficul31]

Some last nitpicks about naming and style and such, but then I will shut up and merge it.

Please do a more thorough review. I can change the PR a hundred times(I think I already have). I want the merge to be perfect and according to every styling guideline.

[reficul31]

@Treora PR updated. Features of the PR

1. Conforming to the guidelines of css styling
2. Adding spaces wherever necessary
3. Changing the variable names to be something more appropriate. Please review the PR. Any changes required shall be implemented immediately. Where should the code be documented? It is imperative to document this portion of code because the implementation is slightly weird(for lack of a better word). Sorry for the delay.

[Treora]

I will just go through it myself again and indeed add a comment to explain the counter. One question: are you okay with waiving all your copyrights on the code, so we can publish it in the public domain? (see e.g. unlicense.org)

[reficul31]

Yes. I am totally fine with the waiving away all the rights on the code. What is the procedure to do so?

[Treora]

No procedure, just that acknowledgement is enough. Thanks for your contribution!

If you want to know what last changes I made, run git show 06bf788 to see the changes I made during merging (GitHub does not seem to show this anywhere). Mainly I changed some tabs into spaces, and decided to rename the actions still for consistency (renaming the argument instead to resolve the naming conflict). Don't think that my coding style is objectively better though, it is just more consistent with the rest of the code.

I realise now that the approach to ignore belated results from simultaneous searches is not so pretty. I may try simplify it some time, which may also simplify the logic for this indicator. One silly quirk now is that the indicator waits for all searches to complete, not just the most recent one, so a slow old query that is already outdated by a new query+new results can keep the loading indicator show for no reason (most noticeably typing a letter and deleting it again can take a relatively long time to restore the unfiltered overview).

Let's move on to more important stuff now, this one has cost more than enough effort already!

[reficul31]

Thanks for all the help, I learned a lot. Really appreciate it. :D

[Chaitya62]

@gerben Do I build on this code and update my PR #56 ?

[Treora]

@Chaitya62: that would be helpful indeed. Rebasing your branch to the current master is always a good idea (look into git rebase if you don't know it; it is a powerful tool, though rebasing can be quite a confusing process)

[Chaitya62]

@Treora I did something similar but it was not git rebase What I did was first pulled all the code from master branch and then used git reset --hard <lastest master commit SHA> although I think rebase is a better option