[Issue 143] Refactor /resources page to use React Query #162
Conversation
src/components/Resources/index.js
Outdated
| </Typography> | ||
| )} | ||
| <br /> | ||
| {loading && !errorMessage ? ( | ||
| {isLoading && !isError ? ( |
There was a problem hiding this comment.
Let's handle the error and loading state like how it's handled above:
if (isLoading) {
return <p>Loading...</p>;
}There was a problem hiding this comment.
Whoops, I just realized that the isLoading && !isError is leftover logic, but since we already have an isLoading on the page we don't need two. So this is the change I made: b5e0335 Is that acceptable?
| @@ -7,4 +7,13 @@ const getResource = async (_key, id) => { | |||
| return data; | |||
| }; | |||
|
|
|||
| export { getResource }; | |||
| const getResources = async searchTerm => { | |||
There was a problem hiding this comment.
It would be nice to have a test for this, mainly for documentation or what we expect to get in return.
There was a problem hiding this comment.
👍 How does this look? 7845982
src/utils/queries.js
Outdated
| const getResources = async searchTerm => { | ||
| const url = | ||
| searchTerm !== '' | ||
| ? `${API_URL}/resources/?search=${searchTerm}` // Empty '' search term will return 0 results |
There was a problem hiding this comment.
It makes sense that an empty search term will return 0 results, but it's a bit of an over reach to query the /resources/ endpoint instead, which would return all or a paginated set of resources IIRC, right? If we're doing this, then the conditional in line 46 of the resource/index page will never get hit
https://github.com/codebuddies/frontend/pull/162/files#diff-cec2f6afae4583cc82d24feda5d79ad6R46
There was a problem hiding this comment.
When the component is initially loaded, I have searchValue initialized to equal '' an empty string but if I search for an empty string, I'll get 0 results:
However, if I type to search '' explicitly in the searchbox or a search term that isn't found, I'll get the expected 0 results:
I'm not sure if this answers your question? Was a bit confused by the question, since the conditional in line 46 in resources/index.js does get hit from what I understand...
There was a problem hiding this comment.
I think I'm mainly finding it odd the way that we're validating the searchTerm in the query method itself. Maybe we should be validating in the frontend before it gets passed to here?
There was a problem hiding this comment.
Hmm... how would we validate in the frontend?
|
Great work overall @lpatmo! |
…ed from the API for some reason h/t Gaurav and Phil
|
@tgrrr Worked w/ @gauravchl to make 5fc473c and b2ad22f which responds to the two suggestions you made! |
|
I've made a couple more comments. One thing I'd like to add handling for is a timeout. Is that something we get for free with React Query? If so, should we add some Toast notifications as another ticket? |
Hmm... not sure if that's something we get from React Query, but yeah, let's revisit in a new ticket! |
| ], | ||
| }; | ||
|
|
||
| axios.get.mockImplementationOnce(() => Promise.resolve({ data })); |
There was a problem hiding this comment.
I find this to be a little weird but unsure how to really fix it. Just leaving this as a comment and perhaps something that we'll need to figure out how to do better.
We're asserting on the response object that we implemented. If the API breaks and that the response object is different, we'll never know in this case.
src/utils/queries.js
Outdated
| const getResources = async searchTerm => { | ||
| const url = | ||
| searchTerm !== '' | ||
| ? `${API_URL}/resources/?search=${searchTerm}` // Empty '' search term will return 0 results |
There was a problem hiding this comment.
I think I'm mainly finding it odd the way that we're validating the searchTerm in the query method itself. Maybe we should be validating in the frontend before it gets passed to here?
Co-authored-by: Angelo Cordon <angelocordon@gmail.com>
…ater on with results &&
…, full results display again
What type of PR is this? (check all applicable)
Context
Closes #143. This PR updates the /resources page to use React Query instead of useEffect.
Screenshots/Recordings (if there are UI changes)
Implementation Details - what was your thought process as you changed the code?
[x] Removed
useEffectin search in favor of React Query[x] Added new getResources function in
utils/queries[x] Removed the avatar part of the cardheader
Related Tickets & Documents (Optional)
Placeholder example: This relates to issue #23.
Filed issue #43 as a next step to do after this PR is merged.
Added tests?
A basic test already exists for the /resources page.
Added to documentation (readme.md or contributing.md)?