WebMemex / webmemex-extension

📇 Your digital memory extension, as a browser extension
https://webmemex.org
Other
208 stars 45 forks source link

Add no result found message issue#53 #56

Closed Chaitya62 closed 7 years ago

Chaitya62 commented 7 years ago

I added a simple if statement and a message style in ./ResultList.jsx and ./ResultList.css

Chaitya62 commented 7 years ago

In the 4th commit I added a new method in Overview class though I am not proud of what I named it are there anyother suggestions? I could not come up with a better option.

Chaitya62 commented 7 years ago

I didn't want to make a new component just for a special case maybe If not function the second approach you pointed out is a feasible solution

Treora commented 7 years ago

|{isFirstVisit &&

This is your digital memory, watch it grow as you browse the web!

} {!isFirstVisit && } |

Two tests? Probably it is not the cleanest solution either, but this became my usual style in this project:

{
    isFirstVisit
        ? <p>...</p>
        : <ResultList .../>
}

But if the lines get too long, perhaps define the message as a string constant up in the file.

Interesting how many styles there are indeed.. and how it seems one of those things where humans are more fussy about than computers. :)

Chaitya62 commented 7 years ago

Made the changes !

Treora commented 7 years ago

Just curious where the original message went now, the one telling there are no results.. ;)

Treora commented 7 years ago

Ok, looks ready to merge? I'll nitpick on some last whitespace myself.

One question still: are you okay with waiving all your copyright on this code, so that the whole code base can be published in public domain? (see unlicense.org)

Chaitya62 commented 7 years ago

Yes no problem at all.

Treora commented 7 years ago

Hmm I just tried it out, and it shows the firstVisitMessage also when there are results but they are just loading still. Did you not notice? This would need a smarter way to test if there are really no items in the database. May be easier to finish this when the result loading indicator (#54) is done.

Chaitya62 commented 7 years ago

Hmm I think I ll figure out some other way to eliminate that !

Chaitya62 commented 7 years ago

Okay made some changes added a isFetching flag have added comments to why I set it to false and true everywhere. Initially it is true as we fetch the db whlle we load overview for the first time.I have added a loading placeholder which can be replaced by a loader later on.

Chaitya62 commented 7 years ago

Yes is it a bad practice?

Chaitya62 commented 7 years ago

A the default is true because we are fetching from the database as soon as we start our app

obsidianart commented 7 years ago

When you start fetching the flag is changed, the initial state is false regardless. The initial state is about what it is, and the fetching is trigger later on in the code. if it's not clear we can talk about it

Chaitya62 commented 7 years ago

Yes intuitively it should be false initially but then the message that only firstVisitors should see appears I guess isLoading is a better name then isFetching but as we discussed I should wait for the loader issue to be resolved

obsidianart commented 7 years ago

wouldn't it be changed to true immediately (stating the search starts straight away)?

Chaitya62 commented 7 years ago

Yes but the component when default was false loads and goes away it look buggy where as when it's true initially the component loads with loading message

obsidianart commented 7 years ago

ok good :)

Chaitya62 commented 7 years ago

How about I add

export  isDatabaseEmpty = ()=>{
    let isEmpty = false;
    db.info().then((result)=>{
        if(result.doc_count === 0){
            isEmpty = true;
        }

    });
    return isEmpty;
}

in pouchdb.js ,to check database is empty or not? The above code won't work due to the async call but can we make a similar method in pouchdb to check the db is empty or not

Treora commented 7 years ago

@Chaitya62: checking the database requires calling an asynchronous function, so it cannot be done while rendering and requires adding extra state. Seems too complicated to me. (your code always returns false by the way, promises don't work this way)

For simplicity, should we perhaps forget about the first-time message again (sorry, I know it was my own suggestion), and just check in the ResultList if there is a query but no results? Then we can think about the first-use message later again, maybe also as part of a more elaborate introduction.

Chaitya62 commented 7 years ago

@Treora just edited my previous message I realised that code won't work ! I ll just add the no result message and update the PR !

Chaitya62 commented 7 years ago

@Treora I think now the PR is ready ! please review it!

Treora commented 7 years ago

Nice and simple, thanks! I made the message a bit more subtle in appearance, as it need not draw much attention. I like the fade-in, good idea!