-
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 # 10 Prevent user from adding duplicate or empty item to the list #34
Conversation
…added a toast error to notify user of empty item name
…ed for emptiness or punctuaction marks
…ed and add only original names to shopping list
…bmit function to return early if validationError is detected
Visit the preview URL for this PR (updated for commit f823aeb): https://tcl-77-smart-shopping-list--pr34-ma-fz-duplicate-or-e-7fmtso5p.web.app (expires Sun, 22 Sep 2024 19:33:55 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
PRAISE: I LOVE how this feature uses the power OF regular expressions or "regex"! I love continuing the use of toasts, rather than using text error messages around the input field. I love that the toast error message is short and clear. NITPICK: I wish the text field would go empty or go blank after receiving the error toast. Now I get the error message, but still have to delete the offending text from the input, before putting in a new item. At the same time though, the input field not being empty DEFINITELY shows that the duplicate item was not added to the 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.
Last PRAISE: I like that there are code comments that explain "the why" throughout this in addition to specific variable names! Great job!
} | ||
|
||
return trimmedInput; | ||
// Return null if no errors are found (input is valid) | ||
return 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.
I like that the validateItemName func was added to the validateTrimmedString.tsx file as opposed to making a brand new file--all of the functions in this file have to do with validation! Nice!
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.
Thank you! Yeah that was definetely a major talking point when we were thinking the logic out. We thought about whether adding the duplicate item validation was expanding the specific validation function too wide, but I think it was the right call indeed :)
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 job with this PR Falak and Maha! I know it wasn't easy at the start in understanding the logic for the duplicate item checking even with spaces, but you both pulled through and done very well in smashing the AC for this! I tested all the use cases for this that included punctuation, single space, characters and symbols, and it all looks good!
|
||
//return error if the item already exists | ||
if (isDuplicateItem(normalizedInputName)) { | ||
return ` ${normalizedInputName} already exists in the 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 job specifying to the user the exact error when duplicate entries are submitted!
@@ -70,7 +70,7 @@ export function App() { | |||
/> | |||
<Route | |||
path="/manage-list" | |||
element={<ManageList listPath={listPath} />} | |||
element={<ManageList listPath={listPath} data={data || []} />} | |||
/> |
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.
suggestion: I don't think wee need to pass data || []
in the data prop, I beleive the data is initialized as an empty array when creating a list. So a list would either have a filled array or an empty array.
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.
Ahhh do you mean we wouldn't need to pass the prop at all or not need to the OR operator to suggest it could be empty?
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.
Sorry that was very unclear 😅 my bad I think you can have it just as data={data}
instead of data={data || []}
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.
Noted!! That makes sense
return ( | ||
<div> | ||
<p> | ||
Hello from the <code>/manage-list</code> page! | ||
</p> | ||
<AddItemForm listPath={listPath} /> | ||
<AddItemForm listPath={listPath} data={data || []} /> |
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 believe this is the same comment on the App.tsx
|
||
// Validates the item name input for a shopping list | ||
export function validateItemName( |
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.
praise: amazing at extending functionality and bringing the new logic and the current logic together to make an awesome new piece of code!
} | ||
|
||
//Remove punctuation marks and normalize input | ||
const punctuationRegex = /[^\p{L}]/gu; |
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.
praise: love the use of the regex, it is something I'm still such a novice to but I know it has a lot of good functionality and it is awesome to see it 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.
It was really awesome to bring it back to life form the dusty attic! lol
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.
alternate regex option: input.toLowerCase().replace(/[^a-z0-9]/g, '')
} | ||
|
||
return trimmedInput; | ||
// Return null if no errors are found (input is valid) |
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: is there a way without breaking everything to have this return trimmedInput
again. I added an item to a test list that was ' hi' and it did trim it and i can't add 'hi' in any different form again I did notice in the DB it is adding the item with the white space to it instead of the trimmedInput
like before?
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.
Screen.Recording.2024-09-15.at.11.28.57.AM.mov
I tried to show that the toast and such does show the trimmed value but when checking the DB there is a single whitespace before the goodbye. Sorry if it is awkward to see 😅
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.
Thats a great point. It was something we explored. I was trying to create a piece of logic with brute force, using if - else statements to return either the error if duplicate item, or return either the normalized input name. The issue was in the addItemForm, the logic is returning the object that is being returned in the validateItemName function. So it reading the normalizedinputname as an error object. Its something I definetely want to explore more. The check for the duplicate error in the addItemForm before the item is added can be refactored..
Description
Users shouldn't be able to add an empty item to their list, or add the same item twice. If they try to do this, we need to show them an error message that explains the problem. This way, we’ll prevent some clutter in their lists.
Related Issue
closes #10
Acceptance Criteria
Type of Changes
enhancement
Updates
Issue.10.mov
Testing Steps / QA Criteria
Apples
, try addingapples
oraPples
ora pples
). This should show an error indicating that the item already exists in the list.