Skip to content
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

Merged
merged 10 commits into from
Sep 15, 2024
Merged
2 changes: 1 addition & 1 deletion src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export function App() {
/>
<Route
path="/manage-list"
element={<ManageList listPath={listPath} />}
element={<ManageList listPath={listPath} data={data || []} />}
/>
Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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 || []}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted!! That makes sense

</Route>

Expand Down
27 changes: 15 additions & 12 deletions src/components/forms/AddItemForm.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { ChangeEvent, FormEvent, useState } from "react";
import { addItem } from "../../api";
import { validateTrimmedString } from "../../utils";
import { addItem, ListItem } from "../../api";
import { validateItemName } from "../../utils";
import toast from "react-hot-toast";

import { useNavigate } from "react-router-dom";

interface Props {
listPath: string | null;
data: ListItem[];
}

enum PurchaseTime {
Expand All @@ -21,7 +22,7 @@ const purchaseTimelines = {
[PurchaseTime.notSoon]: 30,
};

export function AddItemForm({ listPath }: Props) {
export function AddItemForm({ listPath, data: unfilteredListItems }: Props) {
const navigate = useNavigate();

const [itemName, setItemName] = useState("");
Expand All @@ -42,12 +43,16 @@ export function AddItemForm({ listPath }: Props) {
listPath: string,
) => {
e.preventDefault();
const trimmedItemName = validateTrimmedString(itemName);

if (!trimmedItemName) {
toast.error(
"Item name cannot be empty or just spaces. Please enter a valid name.",
);
// Validate the item name input
const validationErrorMessage = validateItemName(
itemName,
unfilteredListItems,
);

// If there's a validation error, show the error and return early
if (validationErrorMessage) {
toast.error(validationErrorMessage);
return;
}

Expand All @@ -62,13 +67,13 @@ export function AddItemForm({ listPath }: Props) {

try {
await toast.promise(
addItem(listPath, trimmedItemName, daysUntilNextPurchase),
addItem(listPath, itemName, daysUntilNextPurchase), // saving original input
{
loading: "Adding item to list.",
success: () => {
setItemName("");
setItemNextPurchaseTimeline(PurchaseTime.soon);
return `${itemName} successfully added to your list!`;
return `${itemName} successfully added to your list!`; // showing original input
},
error: () => {
return `${itemName} failed to add to your list. Please try again!`;
Expand All @@ -79,7 +84,6 @@ export function AddItemForm({ listPath }: Props) {
console.error("Failed to add item:", error);
}
};

const navigateToListPage = () => {
navigate("/list");
};
Expand All @@ -98,7 +102,6 @@ export function AddItemForm({ listPath }: Props) {
name="item"
value={itemName}
onChange={handleItemNameTextChange}
required
aria-label="Enter the item name"
aria-required
/>
Expand Down
41 changes: 36 additions & 5 deletions src/utils/validateTrimmedString.ts
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(
Copy link
Collaborator

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!

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;
Copy link
Collaborator

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!

Copy link
Collaborator Author

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

Copy link
Collaborator

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, '')


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`;
Copy link
Collaborator

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!

}

return trimmedInput;
// Return null if no errors are found (input is valid)
Copy link
Collaborator

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?

Copy link
Collaborator

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 😅

Copy link
Collaborator Author

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..

return null;
Copy link
Collaborator

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!

Copy link
Collaborator Author

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 :)

}
6 changes: 4 additions & 2 deletions src/views/authenticated/ManageList.tsx
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 || []} />
Copy link
Collaborator

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

<ShareListForm listPath={listPath} />
</div>
);
Expand Down