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

Add no result found message issue#53 #56

Merged
merged 1 commit into from Mar 17, 2017

Conversation

Projects
None yet
3 participants
@Chaitya62
Copy link
Contributor

Chaitya62 commented Mar 6, 2017

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


//if no result found display relavant message
if(searchResult.rows.length === 0){
return <p className={styles.message}>

This comment has been minimized.

@obsidianart

obsidianart Mar 6, 2017

Collaborator

unless it is a project decision what do you think of the style (I've seen it most often because it keeps opening close lines)

return (
    <p className={styles.message}>
        No result found
    </p>
)

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 6, 2017

Author Contributor

Sorry didn't get you? is it about the way I wrote the code or the css styling

This comment has been minimized.

@obsidianart

obsidianart Mar 6, 2017

Collaborator

only about indentation, and only if you think my way is better, and only if my way doesn't violate the rest of the project. To show another big project on this style https://github.com/acdlite/redux-router/blob/master/examples/basic/index.js#L49 (line 49)
I normally don't put weight on style but I started like that with the return and it made my life much harder when the project grew

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 6, 2017

Author Contributor

@obsidianart okay got it! but the ul just below that had

   return <ul className={styles.root }>
     .
     .
    </ul>

So I also did the same thing but yes we can surely choose one of the way to write it and include it somewhere in the project description Readme.md or coding guidlines page in the future.

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 6, 2017

Author Contributor

I ll wait for @Treora 's input in this will change it if necessary ! though I completely agree with you @obsidianart and yes I am also picky about coding styles at times it's doesn't matter at the early stage but helps a lot later on.

This comment has been minimized.

@Treora

Treora Mar 6, 2017

Collaborator

This is one of those stylistic choices where I sway back and forth myself. I'd say keep it either way you like now, and we will restyle the whole codebase at some moment if we think it's better. Perhaps combined with a switch to indenting with only two spaces, I just use four because of old Python habits, but it may be less appreciated in the general javascript etiquette.

This comment has been minimized.

@obsidianart

obsidianart Mar 6, 2017

Collaborator

I'm very free about style. I don't care about semicolon, spaces, and many other things. This is one of the few things I had a real issue with but the final choice for me is your.

if(searchResult.rows.length === 0){
return (
<p className={styles.message}>
no result found

This comment has been minimized.

@Treora

Treora Mar 6, 2017

Collaborator

Talking about style, please do indent this line too. Also "no results found" sounds slightly wrong, I would either say "No results", or "No pages found" or something like that.

I now realise it would be good to make a special case for when the database is empty. "No results" is not a nice welcome message on a fresh install. Perhaps move the current code to the Overview instead of the ResultList, then test for both the length of searchResult and the existence of a query, and if both are empty show something nice, e.g. "This is your digital memory, watch it grow as you browse the web!"

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 6, 2017

Author Contributor

Cool I ll make the necessary changes

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 6, 2017

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.

@@ -18,15 +34,18 @@ class Overview extends React.Component {
ref='inputQuery'
>
</input>
<ResultList searchResult={this.props.searchResult} />
{this.createResultList()}

This comment has been minimized.

@obsidianart

obsidianart Mar 6, 2017

Collaborator

this should probably be a component not a function.
I had the same approach at the beginning but the idea of JSX is towards composition.
I don't know if the current code structure allows 2 elements in the same page.
otherwise, the quickest

{isFirstVisit && <p className={styles.message}>This is your digital memory, watch it grow as you browse the web! </p>}
{!isFirstVisit && <ResultList searchResult={this.props.searchResult} />}

Again, that's my opinion, and React has many opinions, so the choice is yours

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 7, 2017

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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 7, 2017

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 7, 2017

Made the changes !

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 7, 2017

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

@Treora
Copy link
Collaborator

Treora left a comment

Looks good; just some small style stuff:

  • keep and add newlines at end of files
  • give more meaningful names to the constants
  • some indentation
this.props.searchResult.rows.length === 0);

//message for first visit
const message = 'This is your digital memory, watch it grow as you browse the web!';

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Too generic name. firstVisitMessage perhaps?


: <ResultList searchResult={this.props.searchResult} />
}

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

I don't know an actually nice style for these things, but for consistency you could keep the indentation that the rest of the code used:

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

And if the <p> component should take multiple lines:

isFirstVisit
    ? (
        <p ...>
            {...}
        </p>
    )
    : <ResultList ...>
@@ -70,4 +70,4 @@
padding: 4px 4px 4px 12px;
color: inherit;
text-decoration: none;
}
}

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

You can leave that newline in its place, I think it was living quite happily there.

@@ -27,6 +27,18 @@ const timeGapToSpaceGap = makeNonlinearTransform({
})

const ResultList = ({searchResult}) => {

//if no results are found
const message = 'no result found';

This comment has been minimized.

@Treora

Treora Mar 8, 2017

Collaborator

Too generic name

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 9, 2017

Yes no problem at all.

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 9, 2017

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

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 9, 2017

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

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 9, 2017

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.

@@ -30,6 +30,10 @@ export function refreshSearch() {
const query = ourState(getState()).query
const oldResult = ourState(getState()).searchResult
filterVisitsByQuery({query}).then(searchResult => {

//set fetching results to true
ourState(getState()).isFetching = true;

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

this seems like direct state manipulation. is it the case?

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 10, 2017

Yes is it a bad practice?

@@ -5,13 +5,16 @@ import * as actions from './actions'
const defaultState = {
searchResult: {rows: []},
query: '',
isFetching: true,

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

why is default true?

}

function setQuery(state, {query}) {
state.isFetching = true; //will start fetching

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

I think the comment is incorrect, this doesn't trigger anything per se

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 10, 2017

Author Contributor

Actaully I think instead of will start fetching. I should elaborate a little while intializing default

return {...state, query}
}

function setSearchResult(state, {searchResult}) {
state.isFetching = false; //will stop fetching

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

I think the comment is incorrect, this doesn't trigger anything per se

This comment has been minimized.

@Chaitya62

Chaitya62 Mar 10, 2017

Author Contributor

What i mean was when we call setSearchResult means the database has returned a value now if the query is changed and we again make it true

This comment has been minimized.

@obsidianart

obsidianart Mar 10, 2017

Collaborator

I understand, I think the problem is around the comment. this is the state of the fetching, not something which trigger a behaviour.

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 10, 2017

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

@obsidianart

This comment has been minimized.

Copy link
Collaborator

obsidianart commented Mar 10, 2017

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

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 10, 2017

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

This comment has been minimized.

Copy link
Collaborator

obsidianart commented Mar 10, 2017

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

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 10, 2017

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

This comment has been minimized.

Copy link
Collaborator

obsidianart commented Mar 10, 2017

ok good :)

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 14, 2017

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

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 15, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 15, 2017

@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 Chaitya62 reopened this Mar 15, 2017

@Chaitya62

This comment has been minimized.

Copy link
Contributor Author

Chaitya62 commented Mar 15, 2017

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

@Chaitya62 Chaitya62 closed this Mar 15, 2017

@Chaitya62 Chaitya62 reopened this Mar 15, 2017

@Treora Treora merged commit 5fbd143 into WebMemex:master Mar 17, 2017

Treora added a commit that referenced this pull request Mar 17, 2017

@Treora

This comment has been minimized.

Copy link
Collaborator

Treora commented Mar 17, 2017

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!

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