-
Notifications
You must be signed in to change notification settings - Fork 1
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' message when results loading #53
Conversation
Refactoring to do: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In console there is a warning: React Hook useEffect has a missing dependency: 'setIsLoading'. Either include it or remove the dependency array. If 'setIsLoading' changes too often, find the parent component that defines it and wrap that definition in useCallback react-hooks/exhaustive-deps
I agree with @willnwhite that this needs refactoring. It would be a better implementation if isLoading
state variable is declared inside useBookSearch.jsx
and returned to App.jsx
This is fixed by commit 6ec0771. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@willnwhite I found a minor flaw while testing. If a query is made & no items are found, the next query if valid will still show up ‘No items found’ while loading/searching. I suggest an onChange handler to reset the error state back to its initial state “” on the next user input/change & before another query is made.
@willnwhite, I can confirm this bug is fixed now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reviewed this PR once again. The minor flaw pointed out by @snrelghgub isn't related to this feature, persistence of the error message until the next search is made was intended like that at the time(open for change), and it exists in the development
branch too. The position of the search error message should be in place of results, and the error message can be improved too - like replacing "No results found!"
with "No results found for your search "previous-search-term"
. Since displaying error message isn't related to this feature(displaying loading message), I believe that shouldn't be reason to not approve this PR.
@willnwhite Since you are using setAreResultsLoading
within the hook both times, it would be better to declare it within the hook and just return the areResultsLoading
value to App.js. This will make the parameters of the useBookSearch
hook consistant and avoid using setAreResultsLoading
as a dependency for useEffect in useBookSearch
. Also, I think organising code like this will make it more modular.
Since the issue with error message raised by @snrelghgub isn't connected with this PR and the feature is working properly, I'm approving this PR.
Thanks @ArunJose, this is a really nice idea for a refactor. |
Pair programming with @Guitarhub786