-
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
Changes from 8 commits
9a55469
7995534
b321236
eb21d28
c92e31e
8ece059
a1deb44
ff10339
654a733
f823aeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,41 @@ | ||
// makes sure the string passed into the function isn't an empty string | ||
export function validateTrimmedString(input: string) { | ||
const trimmedInput = input.trim(); | ||
import { ListItem } from "../api"; | ||
|
||
// 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 commentThe 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! |
||
input: string, | ||
existingItems: ListItem[], | ||
): string | null { | ||
const trimmedInput = input.trim(); //removes leading and trailing whitespaces | ||
|
||
// Condition 1: Check if the input is empty | ||
if (trimmedInput.length === 0) { | ||
return null; | ||
return "Item cannot be empty"; | ||
} | ||
|
||
//Remove punctuation marks and normalize input | ||
const punctuationRegex = /[^\p{L}]/gu; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. alternate regex option: |
||
|
||
const normalizedInputName = trimmedInput | ||
.replace(punctuationRegex, "") | ||
.toLowerCase(); | ||
|
||
//Create a list of normalized existing item names | ||
const normalizedExistingItemNames = existingItems.map((existingItem) => { | ||
return existingItem.name.replace(punctuationRegex, "").toLowerCase(); | ||
}); | ||
|
||
// Condition 2: Check if the normalized input matches any existing item | ||
const isDuplicateItem = (normalizedInputName: string): boolean => { | ||
return normalizedExistingItemNames.some( | ||
(item) => item === normalizedInputName, | ||
); | ||
}; | ||
|
||
//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 commentThe 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! |
||
} | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. question: is there a way without breaking everything to have this return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Screen.Recording.2024-09-15.at.11.28.57.AM.movI 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 commentThe 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.. |
||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 :) |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,19 @@ | ||
import { AddItemForm } from "../../components/forms/AddItemForm"; | ||
import ShareListForm from "../../components/forms/ShareListForm"; | ||
import { ListItem } from "../../api"; | ||
|
||
interface Props { | ||
data: ListItem[]; | ||
listPath: string | null; | ||
} | ||
|
||
export function ManageList({ listPath }: Props) { | ||
export function ManageList({ listPath, data }: Props) { | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is the same comment on the |
||
<ShareListForm listPath={listPath} /> | ||
</div> | ||
); | ||
|
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 ofdata={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