-
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
Delete Shopping List Items #29
Conversation
Visit the preview URL for this PR (updated for commit e80ed42): https://tcl-57-smart-shopping-list--pr29-ap-bm-delete-shoppin-q6pqg26m.web.app (expires Sun, 21 May 2023 23:09:50 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ad3eb6c34c2ec5986fcc218178df5985eb9c9ffb |
* this function must accept! | ||
*/ | ||
/** | ||
* Deletes an individual item from the user's shopping list. |
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 use of JSDoc here!
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, Beth and Amy! Everything is working as expected on my end.
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.
Feature works exactly as expected. Super clean implementation; code is easy to read. I'm a fan of your decision to go with a modal
over an alert
. Great work! 🎉
src/components/ListItem.jsx
Outdated
const dialogRef = useRef(); | ||
|
||
function confirmDelete() { | ||
dialogRef.current.showModal(); |
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.
Mind blown! Had no idea there was a built-in method that handles this sort of behavior so cleanly! Loved your original alert
implementation but I dig this even more!
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 with everyone else here - this is a super cool!
src/components/ListItem.jsx
Outdated
<button type="button" onClick={confirmDelete}> | ||
Delete | ||
</button> | ||
<dialog ref={dialogRef}> |
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.
Awesome use-case for a ref
.
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.
Congrats on being the first TCL team (to my knowledge) to implement a native Dialog element in your shopping list! Great work figuring out this relatively new, tricky DOM API, and implementing a ref
to boot!
src/components/ListItem.jsx
Outdated
}) { | ||
const dialogRef = useRef(); | ||
|
||
function confirmDelete() { |
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 function's name is a little ambiguous. The modal asks our user to confirm their delete action, yes, but confirmDelete
also kind of reads like "we have confirmed the delete; let's go ahead and do it." I think we can call this openModal
, because that's what it does.
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. :-)
src/components/ListItem.jsx
Outdated
<dialog ref={dialogRef}> | ||
<p>Are you sure you want to remove "{name}" from your list?</p> | ||
<button onClick={handleYesClick}>Yes</button> | ||
<button onClick={handleNoClick}>No</button> | ||
</dialog> |
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 job implementing this Dialog element! We have an opportunity to clean it up, as well. Right now, we render a dialog for each individual ListItem. Technically, this is fine. It's unlikely to cause any problems (that I'm aware of), but it also doesn't need to be this way. We could, in theory, have exactly one dialog element, and reuse it for every list item.
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.
My only guess is that it needs to be its own component?
🎉😁👍🏻 Awesome job, team! |
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.
Awesome job. Dialog implementation took it to the next level. Good 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.
Fantastic work, @Amy-Pr and @bethmelmtv! To EJ's point, curious to see how this could be refactored, but this implementation is 🔥!
src/components/ListItem.jsx
Outdated
const dialogRef = useRef(); | ||
|
||
function confirmDelete() { | ||
dialogRef.current.showModal(); |
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 with everyone else here - this is a super cool!
Description
Before this PR, users were not able to delete an item from their shopping list. This PR adds functionality to the application that allows users to delete individual items from their shopping list. This was accomplished by adding a
Delete
button that is rendered in theList
view for each item in the list. If a user chooses to delete an item, they will be prompted with a message asking them to confirm their choice to delete the item. If the user selectsYes
, the item is then deleted on theList
view page and deleted from the firestore database. If the user decides to change their mind , the user can selectNo
option, and the item will not be deleted.Related Issue
Closes #11
Acceptance Criteria
ListItem
component renders a button that allows the user to delete an item from their list when clickeddeleteItem
function inapi/firebase.js
has been filled out, and deletes the item from the Firestore databaseType of Changes
Updates
Before
Before this change, the
List
view UI had no way of allowing the user to delete items from their list that they no longer want to buy.After
After this change, a
delete
button appears next to each list item. Clicking it prompts the user to confirm that they want to remove the item. Clicking "Yes" removes the item from the list. Clicking "No" leaves the item as is.screen-capture.1.webm
Testing Steps / QA Criteria
Home
view and start adding items to your list. Or type in the token id to an existing list.delete
button should appear next to each item.delete
button on a desired item. A windows prompt will appear that asks to confirm the deletion. Click 'Yes.' The alert box should close, and your item should disappear from the list.delete
button. This time click 'No' when the prompt appears. This time the alert box should close, and nothing should change on your list.