-
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] refactor add plant details #25
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 good so far! please note that planting_type should be of the type PlantingTypeEnum
; this may create some typing issues that we'll need to resolve
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 good! a couple more comments
- we should await updateUserPlants
- let's make details not a state
app/add-details/page.tsx
Outdated
|
||
export default function Home() { | ||
const [currentIndex, setCurrentIndex] = useState<number>(1); | ||
const [details, setDetails] = useState<Partial<UserPlants>[]>([]); |
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 make this a variable and not a state?
return; | ||
} | ||
if ( | ||
steps !== 0 && |
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.
is there ever a case where steps===0
? Would you be calling this function if they're not clicking Back/Next
?
app/add-details/page.tsx
Outdated
currentIndex + steps <= plants.length + 1 | ||
) { | ||
if (steps > 0) { | ||
setIsDisabled(true); |
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.
just to confirm, this means if they're moving forward, the Next/Back buttons are disabled, but if they're moving backwards that means they've alr completed the previous page, so they should be able to move from the previous page
app/add-details/page.tsx
Outdated
const updatedDetails = [...details]; | ||
updatedDetails[currentIndex - 1] = { | ||
...updatedDetails[currentIndex - 1], | ||
[field]: value, | ||
plant_id: plants[currentIndex - 1].id, | ||
}; | ||
setDetails(updatedDetails); |
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.
this will need to updated since details
shouldn't be a state
app/add-details/page.tsx
Outdated
} | ||
|
||
function updateDB() { | ||
updateUserPlants(user_id, details); |
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.
This should be async, so we should modify this to:
async function updateDB() {
await updateUserPlants(user_id, details);
router.push('/view-plants');
}
Awaiting ensures that updateUserPlants is completed before rerouting to /view-plants, so that the changes made in the Add Plant Details flow are reflected in /view-plants.
c85195a
to
d2900c2
Compare
What's new in this PR 🧑🌾
Description
Screenshots
How to review
Next steps
Relevant links
Online sources
Related PRs
CC: @ccatherinetan