-
Notifications
You must be signed in to change notification settings - Fork 3
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
Jk bm estimated next date #30
Conversation
…tDaysBetweenDates at dates.js
… documentation to getDaysBetweenDates on dates.js
Visit the preview URL for this PR (updated for commit d5505f7): https://tcl-71-smart-shopping-list--pr30-jk-bm-estimated-next-f2fkai6w.web.app (expires Thu, 14 Mar 2024 10:53:14 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1e7ade9d0f374c4ddb5d7ab6fc541062fc7a1ab4 |
src/api/firebase.js
Outdated
); | ||
const prevEstimate = dateLastPurchased | ||
? getDaysBetweenDates(dateNextPurchased, dateLastPurchased) | ||
: undefined; |
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!
I have a question regarding lines 200-202: In place of undefined, I would have added a calculation getDaysBetweenDates(dateNextPurchased, dateCreated), to get the initial count of days before the first purchase. What is the reason behind setting it to undefined?
@borjaMarti @BikeMouse
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.
Hey Céline!
You are totally right, we updated the implementation to take into account that period instead of passing undefined as the default.
Thanks for the feedback! 😄
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! I think the feature works as expected, and I appreciate that you documented the new function in dates.js.
I'd also like to comment about setting prevEstimate in firebase.js to the value that the user submitted when creating the item as daysUntilNextPurchase:
const prevEstimate = dateLastPurchased
? getDaysBetweenDates(dateNextPurchased, dateLastPurchased)
: getDaysBetweenDates(dateNextPurchased, dateCreated)
Is there a specific reason for keeping it as undefined if dateLastPurchased is null?
🙌🏽 Thanks!
… which is the user input when entering item
…ut until next purchase preference for the first purchases
Hey Viviana! I adressed this on the reply to Céline as well. Thanks for the feedback! ^^ |
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.
It works nicely! 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.
Good work! 👏
Description
For this feature we installed the
@the-collab-lab/shopping-list-utils
dependency so we could import thecalculateEstimate
function, which we use to generate a newdateNextPurchased
each time an item is marked as purchased.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
Updates
Before
After
Testing Steps / QA Criteria
git pull origin jk-bm-estimated-next-date
to pull commit.npm install
to update dependencies.dateNextPurchased
.dateNextPurchased
.