-
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
Issue # 12: Deleting a shopping list item #36
Conversation
…mHandler and wirte up to delete button. Add dialog element to that deleteItemHandler for confirmation. Co-authored-by: Maha Ahmed <eternalmaha@users.noreply.github.com>
… component in ListItem.tsx. It replaces the previous implementation of the deleteItemHandler with a new implementation that uses the window.confirm method to prompt the user for confirmation before deleting the item. If the user confirms the deletion, the deleteItem function is called with the listPath and item as arguments. If the user cancels the deletion, nothing happens. Co-authored-by: eternalmaha
…es the deleteItem function in firebase.ts to use the item's document reference instead of the collection reference. This change improves the efficiency and accuracy of deleting items from the list. Co-authored by eternalmaha
…tes the deleteItem function in firebase.ts to use a try-catch block to try to delete the item with the deleteDoc func from Firestore, or catch the error. Co-authored by eternalmaha
Visit the preview URL for this PR (updated for commit 439f163): https://tcl-77-smart-shopping-list--pr36-ma-rc-feat-delete-it-mfmocw0q.web.app (expires Sun, 29 Sep 2024 02:19:39 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
*/ | ||
//delete an item from the list | ||
export async function deleteItem(listPath: string, item: ListItem) { | ||
const itemDocRef = doc(db, listPath, "items", item.id); |
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.
what happens if itemDocRef is null
?
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.
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.
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.
@RossaMania @adidalal This seems helpful :) We can play around with firebase getting documents methods
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 see what you are saying now! The problem could be that FireBase is not getting the item to begin with. So we need to make sure that the item document is grabbed from firebase properly.
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.
@eternalmaha brought up, since this is the file and functions talking to the firebase, we should use the .exists() method! I haven't seen that since MongoDB!
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 as expected great work Ross and Maha! 👏 👏
src/api/firebase.ts
Outdated
// Let's try the deleteDoc from Firestore. | ||
try { | ||
await deleteDoc(itemDocRef); | ||
alert("Item has been successfully deleted!"); |
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 like the alert to let the user know if an item's successfully deleted! Maybe an enhancement in the future could be to pull the item name so that the alert confirms if "Cheese has been successfully deleted!" but that is a scope creep for the acceptance criteria of this card I suppose.
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.
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.
Yes! We always want to make sure the user knows they are doing the action they're supposed to!
src/components/ListItem.tsx
Outdated
console.log("List Path:", listPath); | ||
|
||
if (isConfirmed) { | ||
deleteItem(listPath as string, 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.
Because deleteItem
re-throws, we'll have to catch it here and handle it too 🙂
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.
Great point!
We wrapped the deleteItem() func call in a try-catch block! In the catch, we have the error being logged into the console! @eternalmaha pointed out we can keep notifying the user simple by doing some sort of alert! I thought we could just tuck an alert in the catch block!
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.
@RossaMania and I were just thinking about the purposes of try-catchs in our deleteItem() call. I was wrapping my head around well "if deleteItem() throws an error as part of the function", do we need to throw an error again when its called? If im understanding correctly, its because there can be an error of course when deleting the item, then there can be an error when the function is being called itself. **as I research more about try-catches and function calls ** We decided to wrap a try catch around the whole function call.
…d item name in alert message.
… block for error handling. If an error occurs during the deletion process, it will be caught and logged to the console. This change improves the robustness of the delete functionality.
… block for error handling and display an alert message on error. Commit co-authored by @eternalmaha
…emDocRef. The deleteItem function in firebase.ts has been updated to handle the case where the itemDocRef is null. This change improves the robustness of the delete functionality by preventing potential errors when trying to delete an item that does not exist.
…. The deleteItem function in firebase.ts has been updated to handle the case where the itemDocRef is null, improving the robustness of the delete functionality. Additionally, the deleteItem function in ListItem.tsx now uses a try-catch block for error handling and displays an alert message on error. If an error occurs during the deletion process, it will be caught and logged to the console. Furthermore, the deleteItem function in firebase.ts has been modified to display the deleted item name in the alert message. This change enhances the overall delete functionality and user experience. Co-authored-by: Maha Ahmed <eternalmaha@users.noreply.github.com>
Description
Users will need to be able to delete items they no longer need to purchase or if the item was added by mistake. We need to avoid cluttering the shopping list, and only display items that fit the users' current state of needs.
Related Issue
closes #12
Acceptance Criteria
Type of Changes
new feature
enhancement
Updates
deleteitems.mov
Testing Steps / QA Criteria