'Loading' message when results loading

[ghost]

Pair programming with @Guitarhub786

[ghost]

Refactoring to do: setIsLoading(true) is currently coupled to scrolling to bottom of page (in handleScroll), but it needs to be coupled to the HTTP request in useBookSearch.

[ArunJose]

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

[ghost]

@ArunJose found a bug: scrolling abruptly to the bottom of the page causes multiple page loads/requests. I've attempted to fix this on this branch, using @ArunJose's fix, and can't reproduce the bug. @ArunJose, can you still reproduce the bug, or is it fixed?

[ghost]

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.

[snrelghgub]

@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.

[ArunJose]

@ArunJose found a bug: scrolling abruptly to the bottom of the page causes multiple page loads/requests. I've attempted to fix this on this branch, using @ArunJose's fix, and can't reproduce the bug. @ArunJose, can you still reproduce the bug, or is it fixed?

@willnwhite, I can confirm this bug is fixed now 👍

[ArunJose]

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.

[ghost]

@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.

Thanks @ArunJose, this is a really nice idea for a refactor.