-
Notifications
You must be signed in to change notification settings - Fork 0
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
Rt el estimate next purchase #26
Conversation
…g calculateEstimate function
…undant code and comments
Visit the preview URL for this PR (updated for commit eb854ac): https://tcl-78-smart-shopping-list--pr26-rt-el-estimate-next-5tohuxv1.web.app (expires Thu, 19 Sep 2024 16:20:56 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c781903507c1507075d7a974036959ddeec29c0a |
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.
Works perfectly 😁👌🏾
And I'm so glad you guys also removed those redundant console.log statements too 😅 |
Hi Eva and Becka, It is working well. Thank you |
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.
Thanks for your work team :) I am happy to see this feature is almost complete ✨
You did a nice job creating the functions in dates.js
and in updateItem
in firebase.js
.
I left a few comments, please reach out if you have any questions or need any clarifications.
src/utils/dates.js
Outdated
@@ -1,3 +1,5 @@ | |||
import { Timestamp } from 'firebase/firestore'; |
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 import can be deleted since not being used
src/utils/dates.js
Outdated
* @returns {number} | ||
*/ | ||
export function getDaysBetweenDates(lastDate, nextDate) { | ||
// const lastDateInMS = lastPurchase.toDate().getTime(); |
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 delete those lines?
itemId, | ||
{ dateLastPurchased, totalPurchases }, | ||
) { | ||
export async function updateItem(listPath, itemId, { totalPurchases }) { |
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.
I discovered when I create an item and mark it as purchased, the dateNextPurchased
is updated to the current date:
I think this is weird bc we are rolling back the dateNextPurchase, and I believe this shouldn't be happening.
This is the item date when it was first created, before being marked as purchased:
What do you think?
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.
Nice job ⚡️
Description
When the item in a list is checked (i.e. purchased), the firebase.js updates the item's dateNextPurchased property. In the updateItem function, the calculateEstimate function is used to calculate the date of the next purchase.
Related Issue
closes #11
Acceptance Criteria
dateNextPurchased
property is calculated using thecalculateEstimate
function and saved to the Firestore databasedateNextPurchased
is saved as a date, not a numbergetDaysBetweenDates
function is exported fromutils/dates.js
and imported intoapi/firebase.js
Type of Changes
New Feature
Updates
There are no UI changes.
Testing Steps / QA Criteria