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

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

Merged
merged 1 commit into from Mar 11, 2017

Conversation

Projects
None yet
3 participants
@reficul31
Copy link
Contributor

reficul31 commented Mar 7, 2017

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
Copy link
Collaborator

Treora left a comment

I wonder why an epic is used for showing the animation, but not for hiding it. And testing for loading==0 seems like inverted logic, make it true/false instead.

Regarding variable names and style: have a look at (css) indentation, newlines at end of files, and name Icon somewhat more specifically. Also the CSS can be more compact.

.ofType(actions.setQuery.getType())
.debounceTime(500)
.map(() => actions.playLoading())

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Listening to query change itself seems slightly wrong. It should listen to whether the query change started a new search. Listening to refreshSearch would however also trigger when the search refreshes because of database changes. Maybe refreshSearchResultsUponQueryChange should trigger another action (e.g. startNewSearch) that activates both refresh and the loading indicator, or it should just emit both those actions (with a mergeMap?) instead of only refreshSearch.

This comment has been minimized.

@reficul31

reficul31 Mar 8, 2017

Author Contributor

I think implementation of a merge map is the appropriate option in this. I will make the changes and send the PR right now.

@@ -1,6 +1,6 @@
import 'rxjs/add/operator/debounceTime'
import 'rxjs/add/operator/map'

import { combineEpics } from 'redux-observable'

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Why is this here?

This comment has been minimized.

@reficul31

reficul31 Mar 8, 2017

Author Contributor

Sorry it was a remainder of the hack I put in place to go about the ...Objects.values(overview.epics) error. I am really sorry I left it there I will fix it right now.

}

function playLoading(state){
return {...state, isFetching:0}

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Should this not be set to 1 or true here? Looks like it is the wrong way around.

This comment has been minimized.

@reficul31

reficul31 Mar 8, 2017

Author Contributor

Yeah initally I got the idea of setting the isFetching to true. But according to the conversation we had on the issue. It was given that multiple searches might take place simultaneously and that is why a counter would be more appropriate. I will change it immediately if required.

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Oh yes I forgot about the need to count them. However then still it should be the other way around: isFetching > 0 would mean the animation should play.

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

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

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 8, 2017

@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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 8, 2017

By refreshSearchResults do you mean refreshSearchResultsUponQueryChange?

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

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 9, 2017

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.

@reficul31 reficul31 force-pushed the reficul31:issue_54 branch 2 times, most recently from 8a66fd1 to 3eee8fc Mar 9, 2017

}

function setQuery(state, {query}) {
return {...state, query}
}

function setSearchResult(state, {searchResult}) {
return {...state, searchResult}
var temp = state.isFetching-1

This comment has been minimized.

@reficul31

reficul31 Mar 9, 2017

Author Contributor

Not sure this is the best way to maintain immutability. If there is a better way do tell I will change immediately.

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 9, 2017

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

@reficul31 reficul31 force-pushed the reficul31:issue_54 branch from 3eee8fc to 042d27c Mar 9, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 9, 2017

@reficul31 reficul31 force-pushed the reficul31:issue_54 branch from 042d27c to 7b99de3 Mar 10, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 10, 2017

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.

@Treora
Copy link
Collaborator

Treora left a comment

Looks good! Some last nitpicks about naming and style and such, but then I will shut up and merge it. :)

<span className={styles.dotone+' '+styles.dot}></span>
<span className={styles.dottwo+' '+styles.dot}></span>
<span className={styles.dotthree+' '+styles.dot}></span>
</div>

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

The rest of the code uses the classnames module, can use it here for consistency:
classNames(styles.dotone, styles.dot)

Also, fix indentation.

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

And let's rename Loader to LoadingIndicator, much clearer.

width: 6%;
height: 9%;
margin:2% auto;
}

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

Replace the relative sizes with some reasonable fixed numbers.

}
.dotone {
animation-iteration-count: infinite;
animation-timing-function: ease-out;

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

Can these two props not be shared in .dot?

@@ -32,6 +33,7 @@ class Overview extends React.Component {
const mapStateToProps = (state) => ({
query: ourState(state).query,
searchResult: ourState(state).searchResult,
loading:ourState(state).isFetching,

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

No reason to change variable name here, loading could just be isFetching. We could also rename it throughout the code to something more elaborate. Perhaps searchIsRunning, or waitingForResults, or something like that?

@@ -18,7 +19,7 @@ class Overview extends React.Component {
ref='inputQuery'
>
</input>
<ResultList searchResult={this.props.searchResult} />
{this.props.loading==0 ? <ResultList searchResult={this.props.searchResult} />: <Loader /> }

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

I would remove ==0 (and swap the two values). Non-zero is truthy, and conceptually the variable is pretty much a boolean, that it keeps count is an implementation detail.

}

.dottwo {
margin-left: 25%;

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

if these are absolute anyway, just left should work, no?

@@ -18,7 +19,7 @@ class Overview extends React.Component {
ref='inputQuery'
>
</input>
<ResultList searchResult={this.props.searchResult} />
{this.props.loading==0 ? <ResultList searchResult={this.props.searchResult} />: <Loader /> }

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

I would remove ==0 (and swap the two values). Non-zero is truthy, and conceptually the variable is pretty much a boolean, that it keeps count is an implementation detail.

This comment has been minimized.

@reficul31

reficul31 Mar 10, 2017

Author Contributor

I am expecting you mean something like

{this.props.loading==0 ? <ResultList searchResult={this.props.searchResult} />: <Loader />  }

But I don't think this will work(it doesn't). The control loop takes a value which is boolean and cannot be int. Unless we make a complex workaround this.

@@ -9,6 +9,8 @@ import { ourState } from './selectors'

export const setQuery = createAction('overview/setQuery')
export const setSearchResult = createAction('overview/setSearchResult')
export const incrementLoadCounter = createAction('overview/incrementLoadCounter')
export const decrementLoadCounter = createAction('overview/decrementLoadCounter')

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

Hmm.. I'm thinking that the fact that they count the amount of running searches could be considered an implementation detail of the reducer logic. The actions could perhaps just be show/hideLoadingIndicator, which expresses their intention, not their implementation. It may be slightly confusing that a hide-action would in some cases not actually hide the indicator, but that seems acceptable to me.

This comment has been minimized.

@reficul31

reficul31 Mar 10, 2017

Author Contributor

Due to name collision may I change it to showLoadIndicator and hideLoadIndicator?

@reficul31 reficul31 force-pushed the reficul31:issue_54 branch from 7b99de3 to 5f05caf Mar 10, 2017

@@ -15,7 +16,19 @@ function setSearchResult(state, {searchResult}) {
return {...state, searchResult}
}

function showLoadIndicator(state){
var temp = state.waitingForResults+1

This comment has been minimized.

@reficul31

reficul31 Mar 10, 2017

Author Contributor

Still doesn't feel the right way to keep immutability. Is there any other way to do this? Something a bit more elegant?

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

Ah yes, thanks for the self-review; I meant to suggest this:

return {
    ...state,
    waitingForResults: state.waitingForResults+1
}
@@ -18,7 +19,7 @@ class Overview extends React.Component {
ref='inputQuery'
>
</input>
<ResultList searchResult={this.props.searchResult} />
{this.props.waitingForResults==0 ? <ResultList searchResult={this.props.searchResult} />: <LoadingIndicator /> }

This comment has been minimized.

@reficul31

reficul31 Mar 10, 2017

Author Contributor

I tried removing the ==0 but it wasn't working. For now I have kept it the same unless we can add a line in the function mapStateToProps which goes something like this

waitingForResults: waitingForResults:ourState(state).waitingForResults==0 ? true:false

But that is essentially doing the same thing

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

Weird? This should be normal javascript:
{this.props.waitingForResults ? <LoadingIndicator /> : <Resultlist searchResult={this.props.searchResult} />}

Are you sure the problem was not something else?

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 10, 2017

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 reficul31 force-pushed the reficul31:issue_54 branch 2 times, most recently from a319279 to 2025afa Mar 10, 2017

@Treora
Copy link
Collaborator

Treora left a comment

As you wish, here's even more nitpicking, mainly just about spaces between interpunction.

display: block;
width: 60px;
height: 60px;
margin:20px auto;

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

<span className={classNames(styles.dotone, styles.dot)}></span>
<span className={classNames(styles.dottwo,styles.dot)}></span>
<span className={classNames(styles.dotthree,styles.dot)}></span>
</div>

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator
return (
    <div...>
        <span....>
   </div>
)```
@@ -32,6 +33,7 @@ class Overview extends React.Component {
const mapStateToProps = (state) => ({
query: ourState(state).query,
searchResult: ourState(state).searchResult,
waitingForResults:ourState(state).waitingForResults,

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

@@ -5,6 +5,7 @@ import * as actions from './actions'
const defaultState = {
searchResult: {rows: []},
query: '',
waitingForResults:0,

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

@@ -15,7 +16,17 @@ function setSearchResult(state, {searchResult}) {
return {...state, searchResult}
}

function showLoadIndicator(state){
return {...state, waitingForResults:state.waitingForResults+1}

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

@@ -15,7 +16,17 @@ function setSearchResult(state, {searchResult}) {
return {...state, searchResult}
}

function showLoadIndicator(state){

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

return {...state, waitingForResults:state.waitingForResults+1}
}

function hideLoadIndicator(state){

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

export default createReducer({
[actions.setQuery]: setQuery,
[actions.setSearchResult]: setSearchResult,
[actions.showLoadIndicator]:showLoadIndicator,
[actions.hideLoadIndicator]:hideLoadIndicator,

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

filterVisitsByQuery({query}).then(searchResult => {
// To show the search ended and to stop the LoadingIndicator
if(showLoadingIndicator){

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

return function (dispatch, getState) {
const query = ourState(getState()).query
const oldResult = ourState(getState()).searchResult
// To show a search was called for and to load the LoadingIndicator
if(showLoadingIndicator){

This comment has been minimized.

@Treora

Treora Mar 10, 2017

Collaborator

spacing

@reficul31 reficul31 force-pushed the reficul31:issue_54 branch from 2025afa to 0423ec1 Mar 11, 2017

@reficul31

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 11, 2017

@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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 11, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 11, 2017

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

@Treora Treora merged commit 0423ec1 into WebMemex:master Mar 11, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 11, 2017

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

This comment has been minimized.

Copy link
Contributor Author

reficul31 commented Mar 11, 2017

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

@Chaitya62

This comment has been minimized.

Copy link
Contributor

Chaitya62 commented Mar 14, 2017

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

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 15, 2017

@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

This comment has been minimized.

Copy link
Contributor

Chaitya62 commented Mar 15, 2017

@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

@reficul31 reficul31 deleted the reficul31:issue_54 branch Jul 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment