-
Notifications
You must be signed in to change notification settings - Fork 1
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
Mark items as purchased #23
Conversation
Visit the preview URL for this PR (updated for commit 0daa6f7): https://tcl-57-smart-shopping-list--pr23-ap-jg-mark-item-as-p-dpq7pnll.web.app (expires Sun, 07 May 2023 04:44:42 GMT) π₯ via Firebase Hosting GitHub Action π Sign: ad3eb6c34c2ec5986fcc218178df5985eb9c9ffb |
β¦ction in firebase.js
src/api/firebase.js
Outdated
const date = data.dateLastPurchased.toDate(); | ||
const currentDate = new Date(); | ||
const timeElapsed = currentDate - date; | ||
isDefaultChecked = timeElapsed < dayInMilliseconds; |
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 looks really clean - nicely done!
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.
Totally agree with @emilysellers! Nicely done. One suggestion for improvement - since some of these day/time variables will probably show up in the future in other places, this would be a good opportunity to move them to a utils.js
file and export them as constants (e.g. export const DAYINMILLISECONDS = ...
. That way they can be reusable and not redefined every time you need them.
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.
everything is working perfectly - great job you guys!!
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.
Really nice work, @Amy-Pr and @jongranados! Your PR is very clear, thorough and descriptive and the feature is working just as you've described. This may be beyond the scope of this issue, but I wonder if we might want to implement any guard against a user's accidental click. If a user checks and then immediately unchecks a box, the date last purchased and total purchases persist in Firestore. The impact on the app's functionality might be minimal, but just thought I'd mention it.
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.
You two had the 'meatier' issue this week, and it was a pleasure to see you think on your feet in office hours. Great job navigating Firestore functions, timestamps, and that defaultChecked property. Excellent work! ππ»ππ
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.
Excellent work, @jongranados and @Amy-Pr! I left a couple of comments about adding some documentation and moving the date/time variables to a constants/utils file so they can be imported separately. I also want to highlight the issue that @emilysellers pointed out regarding what happens if a user checks an item and then unchecks it immediately after. While this is not explicitly in the AC, I would consider this an edge case that will need to be dealt with at some point. Maybe adding a TODO comment and creating a separate issue might be the way to go for now.
src/api/firebase.js
Outdated
const date = data.dateLastPurchased.toDate(); | ||
const currentDate = new Date(); | ||
const timeElapsed = currentDate - date; | ||
isDefaultChecked = timeElapsed < dayInMilliseconds; |
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.
Totally agree with @emilysellers! Nicely done. One suggestion for improvement - since some of these day/time variables will probably show up in the future in other places, this would be a good opportunity to move them to a utils.js
file and export them as constants (e.g. export const DAYINMILLISECONDS = ...
. That way they can be reusable and not redefined every time you need them.
@@ -50,6 +58,20 @@ export function getItemData(snapshot) { | |||
*/ | |||
data.id = docRef.id; | |||
|
|||
let isDefaultChecked; |
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.
Adding some comments here about this logic would be helpful for others working in this component in the future!
* to update an existing item. You'll need to figure out what arguments | ||
* this function must accept! | ||
*/ | ||
export async function updateItem(listId, listItemId) { |
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.
Same comment as above about adding comments to this function. Notice how the other API-related functions in this file are setup - listing params with brief descriptions plus a short summary of what the function actually does. Very useful for maintaining clarity as you iterate and add to this file.
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 the reminder DJ! Going to work on implementing comments more often as I know how important and useful they are.
Description
Before this PR, users were presented with a static view of their grocery list. This PR adds functionality to the application that allows users to interact with their list so that they can keep track of the things they need to buy, and do not need to buy, at any given point in time. This was accomplished by enhancing the user's list view page with a user interface consisting of checkboxes alongside each item on their list that they can checkoff after purchasing. The item remains marked for a 24 hour window after the purchase. And after 24 hours, the application programmatically unchecks the item so that it is available for when the user needs to purchase it again.
Related Issue
Closes #8.
Acceptance Criteria
ListItem
component renders a checkbox with a semantic<label>
.dateLastPurchased
andtotalPurchases
properties on the corresponding Firestore documentupdateItem
function infirebase.js
has been filled out, and sends updates to the firestore database when an item is checked or un-checkedType of Changes
Updates
Before
On the
List
view, there are no checkboxes next to the list items. Looking upState
in the browser dev tools showslastDatePurchased
asnull
andtotalPurchases
as0
. This can also be seen from the Firestore database console (see QA steps below.)After
Each item now has a checkbox. Checking the box updates the
dateLastPurchased
to the current date and increasestotalPurchases
by 1 in the Firestore database. The checkmark automatically unchecks after 24 hours, as demonstrated by rolling backdateLastPurchased
manually in Firestore.screen-capture.1.webm
Testing Steps / QA Criteria
To Check that the dateLastPurchased and totalPurchases have been updated:
Home
or type in the token for an exisiting list.Add Item
view.Application
in your browser dev tools.dateLastPurchased
attribute for that item should saynull
and the totalPurchases should be set to0
.List
view on the Smart Shopping List app.dateLastPurchased
andtotalPurchases
should update to today's date and1
.To Check for the 24-hour automatic uncheck functionality:
dateLastPurchased
on your item by 1 day.List
in the app.