-
Notifications
You must be signed in to change notification settings - Fork 44
feat: create app #29
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
base: main
Are you sure you want to change the base?
feat: create app #29
Conversation
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.
Hi @task-dev-alt ,
Great work mate! Thank you a lot for the effort you put in this PR!
Could you please check my comments when you have some time?
- How come you used tailwind? Are you familiar with alternatives?
- I notice you chose to use React Query for state management instead of Redux or similar solutions. Could you explain your thought process behind this decision and any trade offs you considered?
- The current favorite cats implementation fetches all favorites at once. How would you modify this to handle thousands of favorites while maintaining good performance?
- How could we improve the performance of our page load time regarding images? Are there any ways the API gives us to achieve that?
Again many thanks for your work!
@@ -0,0 +1,22 @@ | |||
const apiKey = | |||
import.meta.env.VITE_API_KEY ?? | |||
"live_TvpSUchzwViDMtV85zilyB9NY9RH8Zz0atuDHnSP9X5LycsRtLQFoCh52EhzfwrQ"; |
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.
Are there any concerns on adding this value in here?
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.
Normally I'd avoid hardcoding the key, but in this demo I chose to add it for the following reasons:
- there's a rate limit in the API,
- the key would anyway be visible in the network tab and available in the bundled code,
- the app is entirely client side.
Client side apps can't hide things, so using an environment variable would do little here, in my opinion.
What do you think?
...(breedId && { breed_id: breedId }), | ||
}); | ||
|
||
return useInfiniteQuery({ |
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.
How come you've decided to use useInfiniteQuery
in here? What are the reasons behind this implementation?
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.
Showing a list of 10 random images with a button to load more is a great case for useInfiniteQuery
, as the newly fetched items are appended to the previous ones on every fetch. The React Query docs also suggest this.
BreedFilters and Breeds were storing the filters state twice. State was moved to useBreedsState and handlers are being passed to BreedFilters. Also rearranged the useBreedsState a bit to make it more readable.
Hello @gpositive, Thanks for the nice words and for the thought provoking comments! I pushed a few updates today but I'll stop since you started reviewing it.
Thanks again for your thoughtful comment! EDIT: I'll reply to the other comments after the weekend. Have a nice one! |
Hello @gpositive, I wanted to write a bit about the favourite cats optimization after giving it some thought. The API has some limitations when it comes to the favourite cats:
If any of those 2 was possible, we'd be able to paginate the list request and also be able to show the favourite status in each individual cat in the other 2 pages. However, now, it's necessary to fetch all favourite cats without paginating. Ideally, this would be solved in the backend to allow either of the above. But, if we really had to solve this on the client side for thousands of records, I'd do one or more of the following:
|
No description provided.