Skip to content

Managing listPath's impossible null state #37

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Managing listPath's impossible null state #37

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9e8428
List path is no longer optional where applicable
tannaurus Sep 22, 2024
8bbd8fc
Merge branch 'main' into tg/list-path
tannaurus Sep 22, 2024
aa3a341
Extra clean up post-merge
tannaurus Sep 22, 2024
c1a8844
Only suggesting list is empty when list is selected
tannaurus Sep 22, 2024
74b3e38
Deleted 'your list is ready'
tannaurus Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions src/components/ListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { moreThan24HoursPassed, getDaysBetweenDates } from "../utils";

interface Props {
item: ListItem;
listPath: string | null;
listPath: string;
}
interface None {
kind: "none";
Expand Down Expand Up @@ -66,11 +66,6 @@ export function ListItemCheckBox({ item, listPath }: Props) {
// Temporarily store the updated check state
setUpdatedCheckState({ kind: "set", value: newCheckedState });

if (!listPath) {
toast.error("Error: listPath is missing or invalid.");
return;
}

try {
await toast.promise(updateItem(listPath, item), {
loading: `Marking ${item.name} as purchased!`,
Expand Down
145 changes: 68 additions & 77 deletions src/components/forms/AddItemForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import toast from "react-hot-toast";
import { useNavigate } from "react-router-dom";

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

Expand Down Expand Up @@ -90,82 +90,73 @@ export function AddItemForm({ listPath, data: unfilteredListItems }: Props) {

return (
<section>
{listPath && (
<>
<form onSubmit={(e) => handleSubmit(e, listPath)}>
<h3>First, add your item!</h3>
<label htmlFor="item-name">
Item:
<input
id="item-name"
type="text"
name="item"
value={itemName}
onChange={handleItemNameTextChange}
aria-label="Enter the item name"
aria-required
/>
</label>
<br />
<h3>Next, pick when you plan on buying this item again!</h3>
<fieldset>
<legend>When to buy:</legend>
<label htmlFor={PurchaseTime.soon}>
<input
type="radio"
id={PurchaseTime.soon}
name="when-to-buy"
value={PurchaseTime.soon}
required
onChange={() => handleNextPurchaseChange(PurchaseTime.soon)}
checked={itemNextPurchaseTimeline === PurchaseTime.soon}
aria-label={`Set buy to soon, within ${purchaseTimelines[PurchaseTime.soon]} days`}
/>
Soon -- Within {purchaseTimelines[PurchaseTime.soon]} days!
</label>
<br />
<label htmlFor={PurchaseTime.kindOfSoon}>
<input
type="radio"
id={PurchaseTime.kindOfSoon}
name="when-to-buy"
value={PurchaseTime.kindOfSoon}
required
onChange={() =>
handleNextPurchaseChange(PurchaseTime.kindOfSoon)
}
checked={itemNextPurchaseTimeline === PurchaseTime.kindOfSoon}
aria-label={`Set buy to kind of soon, within ${purchaseTimelines[PurchaseTime.kindOfSoon]} days`}
/>
Kind of soon -- Within{" "}
{purchaseTimelines[PurchaseTime.kindOfSoon]} days!
</label>
<br />
<label htmlFor={PurchaseTime.notSoon}>
<input
type="radio"
id={PurchaseTime.notSoon}
name="when-to-buy"
value={PurchaseTime.notSoon}
required
onChange={() =>
handleNextPurchaseChange(PurchaseTime.notSoon)
}
checked={itemNextPurchaseTimeline === PurchaseTime.notSoon}
aria-label={`Set buy to not soon, within ${purchaseTimelines[PurchaseTime.notSoon]} days`}
/>
Not soon -- Within {purchaseTimelines[PurchaseTime.notSoon]}{" "}
days!
</label>
</fieldset>
<button type="submit" aria-label="Add item to shopping list">
Submit Item
</button>
</form>
<h4>Let&apos;s go look at your list!</h4>
<button onClick={navigateToListPage}>{"View List"}</button>
</>
)}
<form onSubmit={(e) => handleSubmit(e, listPath)}>
<h3>First, add your item!</h3>
<label htmlFor="item-name">
Item:
<input
id="item-name"
type="text"
name="item"
value={itemName}
onChange={handleItemNameTextChange}
aria-label="Enter the item name"
aria-required
/>
</label>
<br />
<h3>Next, pick when you plan on buying this item again!</h3>
<fieldset>
<legend>When to buy:</legend>
<label htmlFor={PurchaseTime.soon}>
<input
type="radio"
id={PurchaseTime.soon}
name="when-to-buy"
value={PurchaseTime.soon}
required
onChange={() => handleNextPurchaseChange(PurchaseTime.soon)}
checked={itemNextPurchaseTimeline === PurchaseTime.soon}
aria-label={`Set buy to soon, within ${purchaseTimelines[PurchaseTime.soon]} days`}
/>
Soon -- Within {purchaseTimelines[PurchaseTime.soon]} days!
</label>
<br />
<label htmlFor={PurchaseTime.kindOfSoon}>
<input
type="radio"
id={PurchaseTime.kindOfSoon}
name="when-to-buy"
value={PurchaseTime.kindOfSoon}
required
onChange={() => handleNextPurchaseChange(PurchaseTime.kindOfSoon)}
checked={itemNextPurchaseTimeline === PurchaseTime.kindOfSoon}
aria-label={`Set buy to kind of soon, within ${purchaseTimelines[PurchaseTime.kindOfSoon]} days`}
/>
Kind of soon -- Within {purchaseTimelines[PurchaseTime.kindOfSoon]}{" "}
days!
</label>
<br />
<label htmlFor={PurchaseTime.notSoon}>
<input
type="radio"
id={PurchaseTime.notSoon}
name="when-to-buy"
value={PurchaseTime.notSoon}
required
onChange={() => handleNextPurchaseChange(PurchaseTime.notSoon)}
checked={itemNextPurchaseTimeline === PurchaseTime.notSoon}
aria-label={`Set buy to not soon, within ${purchaseTimelines[PurchaseTime.notSoon]} days`}
/>
Not soon -- Within {purchaseTimelines[PurchaseTime.notSoon]} days!
</label>
</fieldset>
<button type="submit" aria-label="Add item to shopping list">
Submit Item
</button>
</form>
<h4>Let&apos;s go look at your list!</h4>
<button onClick={navigateToListPage}>{"View List"}</button>
</section>
);
}
8 changes: 2 additions & 6 deletions src/components/forms/ShareListForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getUser } from "../ProtectedRoute";
import toast from "react-hot-toast";

interface Props {
listPath: string | null;
listPath: string;
}

const ShareListForm = ({ listPath }: Props) => {
Expand All @@ -19,14 +19,10 @@ const ShareListForm = ({ listPath }: Props) => {

const handleInvite = async (
e: FormEvent<HTMLFormElement>,
listPath: string | null,
listPath: string,
) => {
e.preventDefault();

if (!listPath) {
return;
}

try {
await toast.promise(shareList(listPath, currentUser, emailName), {
loading: "sharing list with existing user",
Expand Down
8 changes: 4 additions & 4 deletions src/utils/validateTrimmedString.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,21 @@ export function validateItemName(
input: string,
existingItems: ListItem[],
): string | null {
const trimmedInput = input.trim(); //removes leading and trailing whitespaces
const trimmedInput = input.trim(); // removes leading and trailing white spaces

// Condition 1: Check if the input is empty
if (trimmedInput.length === 0) {
return "Item cannot be empty";
}

//Remove punctuation marks and normalize input
// Remove punctuation marks and normalize input
const punctuationRegex = /[^\p{L}]/gu;

const normalizedInputName = trimmedInput
.replace(punctuationRegex, "")
.toLowerCase();

//Create a list of normalized existing item names
// Create a list of normalized existing item names
const normalizedExistingItemNames = existingItems.map((existingItem) => {
return existingItem.name.replace(punctuationRegex, "").toLowerCase();
});
Expand All @@ -31,7 +31,7 @@ export function validateItemName(
);
};

//return error if the item already exists
// Return error if the item already exists
if (isDuplicateItem(normalizedInputName)) {
return ` ${normalizedInputName} already exists in the list`;
}
Expand Down
20 changes: 14 additions & 6 deletions src/views/authenticated/List.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,23 @@ export function List({ data: unfilteredListItems, listPath }: Props) {
.sort(comparePurchaseUrgency);
}, [searchTerm, unfilteredListItems]);

const Header = () => {
return (
<p>
Hello from the <code>/list</code> page!
</p>
);
};

if (!listPath) {
Copy link
Collaborator

@bbland1 bbland1 Sep 22, 2024

Choose a reason for hiding this comment

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

I see that the Header handles the retruning early if listPath is null or falsy so that the components that require listPath to be defined don't have to do the checks themselves...

I think I am trying to wrap my head around why do that check here in the List & ManageList instead of higher in the App, would it have over complicated the logic or does it make conceptual sense to put it here for both of them?

Copy link
Collaborator Author

@tannaurus tannaurus Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We probably could have made those routes re-direct if listPath was null, but that would be changing existing behavior in the app. We've been displaying these routes + header if there was no listPath, and I didn't want to take the liberty of changing that because I think that should be the team's decision. It could be thought of as part of the project you had mentioned some interest in; there's a good amount of overlap between "what does this look like when you're not-signed in" and "what does it look like when you first sign in"

Might be something worth raising to the team!

Copy link
Collaborator

@bbland1 bbland1 Sep 23, 2024

Choose a reason for hiding this comment

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

That makes total sense, I didn't think about how we have had specific functionality and how that could change the app. Thinking about how the overall process/usage flow of the app is a consideration in where the listPath can be null and can't be.

return <Header />;
}

// Early return if the list is empty
if (unfilteredListItems.length === 0) {
return (
<>
<p>
Hello from the <code>/list</code> page!
</p>
<Header />
<section>
<h2>Your list is ready!</h2>
<h3>
Expand All @@ -49,9 +59,7 @@ export function List({ data: unfilteredListItems, listPath }: Props) {
// Main content when list is not empty
return (
<>
<p>
Hello from the <code>/list</code> page!
</p>
<Header />

<div>
<section>
Expand Down
14 changes: 12 additions & 2 deletions src/views/authenticated/ManageList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,21 @@ interface Props {
}

export function ManageList({ listPath, data }: Props) {
return (
<div>
const Header = () => {
Copy link
Collaborator

@bbland1 bbland1 Sep 23, 2024

Choose a reason for hiding this comment

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

I like this like internal component usage in creating the header. I've never really done that usually just the full pull out of a component but this is a cool thing to keep top of mind.

Copy link
Collaborator Author

@tannaurus tannaurus Sep 24, 2024

Choose a reason for hiding this comment

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

I personally do things like this when it's a super simple thing, like a few lines that happen to repeated a few times in a given component. If it becomes any more complex, and especially if there are hooks invalid, it's much butter to move it out of the parent component. When it's this simple though, retaining the context of the parent is really nice (I don't have to name it "ManageListHeader" because it's defined inside "ManageList")

return (
<p>
Hello from the <code>/manage-list</code> page!
</p>
);
};

if (!listPath) {
return <Header />;
}

return (
<div>
<Header />
<AddItemForm listPath={listPath} data={data || []} />
<ShareListForm listPath={listPath} />
</div>
Expand Down