-
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
Ar mp mark item purchased #26
Conversation
Co-authored-by: Allison Randel <[email protected]>
Co-authored-by: Allison Randel <[email protected]>
…stPurchased field updates when user clicks/checks checkbox next to an item
…ckbox to uncheck itself when calcTime is >= 24 hours
…ased in firebase. Item will stay unchecked and not let you check it again unless you manually change back the dateLastPurchased.
Visit the preview URL for this PR (updated for commit c2af263): https://tcl-76-smart-shopping-list--pr26-ar-mp-mark-item-purc-g415r0va.web.app (expires Tue, 17 Sep 2024 17:32:34 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 512b1a88be8ae05fd3e727b99332819df760271d |
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 cool implementation :) Was able to check and uncheck items and see properties in the database, and saw the update on click.
Though when testing the 24 hours feature (In firebase, I changed dateLastPurchased
to the previous day and a minute after), the page updated after a refresh, but I noticed isChecked
in the database stayed equal to true
. It could be how I tested it, so I'm curious if anyone also had this happen. It does update in LocalStorage as well!
src/components/ListItem.jsx
Outdated
if (is24HoursPassed) { | ||
// console.log('Unchecking item because 24 hours have passed'); | ||
setChecked(false); | ||
localStorage.setItem(`checked-${item.name}`, JSON.stringify(false)); |
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.
nitpick ( non-blocking ): Maybe passing the checked state into since its updating anyway.
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.
src/components/ListItem.jsx
Outdated
|
||
// Handle checkbox change (when user clicks on it) | ||
const handleChange = () => { | ||
const newCheckedState = !checked; |
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.
question: should the state of the item being checked depend only on the checked state in the app/firestore or should that also be dependent on the date?
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.
Possible solution: totalPurchases increments by 1 when the item unchecks itself.
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.
@arandel1 from the AC (and at least for now), the box once checked, should probably stay checked for 24 hours, as in the user won't be able to uncheck it.
Perhaps we can create another ticket for the case of accidentally checking off an item.
src/components/ListItem.jsx
Outdated
const has24HoursPassed = (dateLastPurchased) => { | ||
const purchaseTime = dateLastPurchased.getTime(); // Time in milliseconds | ||
const currentTime = new Date().getTime(); // Current time in milliseconds | ||
const oneDayInMs = 24 * 60 * 60 * 1000; // 24 hours in milliseconds |
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.
nit (non-blocking): as this is a constant, consider renaming in uppercase snake case. Note: this constant exists in dates.js
@@ -6,6 +6,7 @@ import { | |||
doc, | |||
onSnapshot, | |||
updateDoc, | |||
increment, |
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 find with this increment
function
}); | ||
|
||
// Function to check if 24 hours have passed | ||
const has24HoursPassed = (dateLastPurchased) => { |
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 function naming! I'm always a fan of easily readable code!
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.
Agree!
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.
Looks good so far, but a couple of bugs to fix
…here I think the update increment function should go.
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 work on this challenging ticket! I appreciate how readable the variable names are. After testing, it met all the criteria. Just have one small nitpick, like Devin mentioned, the totalPurchase updates every time I check and uncheck, even if it's incidental. I'm curious to learn about what is a better way to capture only valid selections—perhaps adding a confirm button after each item?
…check after 24 hours, and dateLastPurchased turns null. Need to remove local storage piece.
… relies on dateLastPurchased and not local storage, user can check a checkbox and dateLastPurchased updates with a timestamp and totalPurchase increments by 1 and the checkbox remains checked and unclickable so the user cannot update the checkbox etc.
src/api/firebase.js
Outdated
* this function must accept! | ||
*/ | ||
// User can update items in their list by checking the checkbox associated with an item, and the totalPurchase field increments by 1 in Firebase. | ||
// The checkbox unchecks 24 hours after the box is checked. |
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.
nitpick ( non-blocking ): Technically, this second sentence isn't relevant to this piece of code.
src/components/ListItem.jsx
Outdated
// Destructure item props | ||
const { name, dateLastPurchased } = item; | ||
|
||
// Keeping [checked, setChecked] state, but removing any calls to local storage | ||
const [checked, setChecked] = useState(false); | ||
|
||
// Function to check if 24 hours have passed: Changed variable tagging. |
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.
nitpick ( non-blocking ): These comments are more notes to yourself than something that will likely be helpful in the long run.
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.
Removed notes
src/utils/dates.js
Outdated
|
||
// getDaysBetweenDates: We need to write a function that calculates current day - dateLastPurchased to calculate the number of days between the current day and the date last purchased | ||
|
||
// if dateLastPurchased = null, then dateNextPurchased = null | ||
// dateNextPurchased = days until you need this item again + dateLastPurchased |
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.
nitpick ( non-blocking ): These also look like they may have been notes left over :)
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.
Removed notes
package.json
Outdated
"@emotion/react": "^11.13.3", | ||
"@emotion/styled": "^11.13.0", | ||
"@mui/material": "^6.0.2", |
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 (non-blocking): Normally, I'd say to be sure to remove packages you decide not to use with npm uninstall
, but I know we're using the same package in the other branch so it's all good 👍
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.
Looks great! Applauding y'alls tenacity on this one! 👏 Woohoo!
Description
The app now keeps track of how many times a user purchases an item:
Related Issue
closes 9
Acceptance Criteria
Type of Changes
feature
enhancement
Updates
Before
After
Testing Steps / QA Criteria
npm i
to ensure MUI is installed properlydateLastPurchased
field update to the time you checked the checkbox (or, the current time)totalPurchases
increase by 1.To watch the checkbox uncheck itself in real time:
dateLastPurchased
field by clicking the pencil next to the field: Edit the field to the day + one minute before the current time (You can mess around with this!)