search

chingu-voyages / v24-geckos-team-02

Books Plus | Voyage-24 | https://chingu.io/
GNU General Public License v3.0
1 stars 1 forks source link

'Loading' message when results loading #53

Closed ghost closed 3 years ago

ghost commented 3 years ago

Pair programming with @Guitarhub786

ghost commented 3 years ago

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.

ghost commented 3 years ago

@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 commented 3 years ago

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.

ArunJose commented 3 years ago

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

ghost commented 3 years ago

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