-
Notifications
You must be signed in to change notification settings - Fork 2
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
[feat] implement routing to plant page #38
[feat] implement routing to plant page #38
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.
looking great! the functionality is fantastic 🤩 A couple big changes to consider
- Create an OwnedPlant type and track a state of OwnedPlant[] instead of a map of UUIDs
- split handlePlantCardClick into 2 functions that take in a OwnedPlant and a Plant to handle clicking on User PlantCards and PlantCards respectively
- remove InlineDiv and handle clicks all within PlantCard
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.
looking so great!! some final things but otherwise ready to merge!
app/view-plants/page.tsx
Outdated
}, []); | ||
|
||
useEffect(() => { | ||
const fetchData = async () => { |
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.
can we directly just move fetchUserPlants into the useEffect instead of using fetchData as another wrapper?
so it could look like
useEffect(() => {
const fetchUserPlants = async () => {
const fetchedUserPlants = await getUserPlantsByUserId(user_id);
const ownedPlants: OwnedPlant[] = await Promise.all(
fetchedUserPlants.map(async userPlant => {
const plant = await getMatchingPlantForUserPlant(userPlant);
return {
userPlantId: userPlant.id,
plant,
};
}),
);
setOwnedPlants(ownedPlants);
}, []); // eventually when ProfileContext is connected, userId would be in the dependency list.
See IJP for an example of using queries in the useEffect
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.
note: since the queries (both getUserPlantsByUserId
and getMatchingPlantForUserPlant
) throw an error on error, we may want to use a try / catch block here and wherever a query is called to handle the error, i.e. console.error() it in the future (see IJP's example above)
…to track selected plants in page
ce3361d
to
e694946
Compare
What's new in this PR 🧑🌾
Description
Screenshots
How to review
Next steps
Relevant links
Online sources
Related PRs
CC: @ccatherinetan