the-collab-lab / tcl-28-smart-shopping-list

1 stars 3 forks source link

Kelsey and Sandy - Avoid Duplicate Items #24

Closed sandaiiyahh closed 3 years ago

sandaiiyahh commented 3 years ago

For an example of how to fill this template out, see this Pull Request.

Description

AddItems Component

Related Issue

closes #6

Acceptance Criteria

Type of Changes

Type
:bug: Bug fix
:sparkles: New feature
:hammer: Refactoring
:100: Add tests
:link: Update dependencies
:scroll: Docs

Updates

Before

Screen Shot 2021-07-27 at 3 09 57 PM

After

Screen Shot 2021-07-27 at 4 07 32 PM

Testing Steps / QA Criteria

github-actions[bot] commented 3 years ago

Visit the preview URL for this PR (updated for commit 1c7bb54):

https://tcl-28-shopping-list--pr24-ks-sd-avoid-duplicat-e1vgc9k2.web.app

(expires Thu, 05 Aug 2021 23:28:54 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

isaabutaa commented 3 years ago

Nice job Kelsey and Sandy! I pulled down the branch and tried it, and it works as you explained it would. I tried a number of different variations of chicken (chicken, Chicken, chicken!, ch!.-icken'), and it checks out. 🐔

In answer to your question, I wonder if it would be helpful to have a standardized format throughout? Like maybe we could ensure that all items saved in the database are lowercase. When we add an item using the variable itemTemplate, I wonder if we could have name: formattedItem, instead of name: item so that it saves the formatted version. But regardless, I think the code here works great as is. 🙂 Nice job!

sandaiiyahh commented 3 years ago

Nice work Kelsey and Sandy! Code is easy to read. The Firestore side of the code is similar to ours, and the usage of Regex to format the input is very nice! I tried creating a list and tried to add duplicate items. It showed me the alert message. 🥳

Just a suggestion, I think it would be better if we didn't throw an error in the console.

Oh true on the console log, I am not sure what is best practice! What do you think about that Kelsey?

sandaiiyahh commented 3 years ago

Nice job Kelsey and Sandy! I pulled down the branch and tried it, and it works as you explained it would. I tried a number of different variations of chicken (chicken, Chicken, chicken!, ch!.-icken'), and it checks out. 🐔

In answer to your question, I wonder if it would be helpful to have a standardized format throughout? Like maybe we could ensure that all items saved in the database are lowercase. When we add an item using the variable itemTemplate, I wonder if we could have name: formattedItem, instead of name: item so that it saves the formatted version. But regardless, I think the code here works great as is. 🙂 Nice job!

Yay, glad they all worked out with the regex! 🐔Yeah, the standardization would be ideal, being that it could eliminate having to check for lowercase each time 🤔

ksiman14 commented 3 years ago

Nice work Kelsey and Sandy! Code is easy to read. The Firestore side of the code is similar to ours, and the usage of Regex to format the input is very nice! I tried creating a list and tried to add duplicate items. It showed me the alert message. 🥳 Just a suggestion, I think it would be better if we didn't throw an error in the console.

Oh true on the console log, I am not sure what is best practice! What do you think about that Kelsey?

Hmm, it would seem strange to have a catch statement without any console log, maybe we just log our message (i.e. "Error adding item to list" and not the full error?

sandaiiyahh commented 3 years ago

Nice work Kelsey and Sandy! Code is easy to read. The Firestore side of the code is similar to ours, and the usage of Regex to format the input is very nice! I tried creating a list and tried to add duplicate items. It showed me the alert message. 🥳 Just a suggestion, I think it would be better if we didn't throw an error in the console.

Oh true on the console log, I am not sure what is best practice! What do you think about that Kelsey?

Hmm, it would seem strange to have a catch statement without any console log, maybe we just log our message (i.e. "Error adding item to list" and not the full error?

Oh the catch error is fine. The throw error is the one logging to the console: const err = new Error('You have already added this item!'); throw err; Maybe we don't need this since there is already that catch error to fall back on?