Issue 5. Add Items to list #20

sillytsundere · 2024-02-12T23:10:30Z

Description

These code changes enable users to add items to their shopping lists via a form on the ManageLists view. The form uses semantic html elements to adhere to accessibility standards.

Related Issue

Closes #5

Acceptance Criteria

UI-related tasks:

The ManageList view displays a form that allows them to enter the name of the item and select how soon they anticipate needing to buy it again. There should be 3 choices for this:
- “Soon”, corresponding to 7 days
- “Kind of soon”, corresponding to 14 days
- “Not soon”, corresponding to 30 days
The input that accepts the name of the item has a semantic label element associated with it
The user can submit this form with both the mouse and the Enter key
When the user submits the form, they see a message indicating that the item either was or was not saved to the database.

Data-related tasks:

The console.log in the addItem function in src/api/firebase.js is replaced with a function that adds the new document to the Firestore database. That function will be imported from the firebase/firestore module.
The user’s soon/not soon/kind of soon choice is used to calculate nextPurchasedDate

Type of Changes

enhancement

Updates

Before

After

Testing Steps / QA Criteria

git pull
git checkout ja-pc-feature-add-item
npm ci
npm run start
Navigate to Manage List page to view form
Enter item and when you will next purchase item into form fields, submit form, view alert informing item was successfully saved
Navigate to List page to view newly added item

…e item to database on list

github-actions · 2024-02-12T23:11:23Z

Visit the preview URL for this PR (updated for commit 078f475):

https://tcl-73-smart-shopping-list--pr20-ja-pc-feature-add-it-txa4cj02.web.app

_{(expires Fri, 23 Feb 2024 23:47:09 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 99eed22fcca8bd9874e77f7b7f7d1eeb554a1666}

rachelspencer · 2024-02-13T21:31:11Z

Hi @sillytsundere and @jaguilar89!
This looks so neat and tidy! I pulled this branch down and ran the app.

I noticed I could submit an item when the input field and radio buttons were unselected. I could also submit an item with only a name and no date requirement, and also submit with no name and only a date requirement. When doing this I got the success alert.

To overcome this I added a <required> attribute to the <input> tags.

To enhance accessibility I also added an <id> tag who's value matches the <forHTML> attribute already existing on the <label> tags.

I could enter an item using the mouse and enter key! I am not sure if you want to enhance the accessibility more and add functionality for a user to select the radio buttons by using the tab and enter key also. Would definitely be extra challenge but might be fun!

The new item const on line 19 isn't used, not sure if you want to take it out to clean up the console.log, or if you have plans to use it later.

Great work!!

PS - I'm not sure if we should be seeing the items in Firebase DB with the extra functionality added for this issue. I couldn't see them, but I'm also still getting the hang of Firebase.

rachelspencer

Hey @jaguilar89 and @sillytsundere,

This looks so cool!

I added some comments on the changes I made on the branch, as well as a couple additional non-blocking suggestions that I didn't implement.

rachelspencer · 2024-02-15T00:10:39Z

src/views/ManageList.jsx

+						name="itemName"
+						type="text"
+						id="item"
+						required


added required so form field can't be left blank on submit

rachelspencer · 2024-02-15T00:11:36Z

src/views/ManageList.jsx

+								name="daysUntilNextPurchase"
+								type="radio"
+								id="soon"
+								required


added required so form field can't be left blank on submit

rachelspencer · 2024-02-15T00:12:08Z

src/views/ManageList.jsx

+								name="daysUntilNextPurchase"
+								type="radio"
+								id="kind-of-soon"
+								required


added required so form field can't be left blank on submit

rachelspencer · 2024-02-15T00:13:38Z

src/views/ManageList.jsx

+								onChange={handleChangeDate}
+								name="daysUntilNextPurchase"
+								type="radio"
+								id="not-soon"


added id attribute to match forHTML attribute on <label> for accessibility

rachelspencer · 2024-02-15T00:13:51Z

src/views/ManageList.jsx

+								onChange={handleChangeDate}
+								name="daysUntilNextPurchase"
+								type="radio"
+								id="kind-of-soon"


added id attribute to match forHTML attribute on <label> for accessibility

rachelspencer · 2024-02-15T00:13:57Z

src/views/ManageList.jsx

+								onChange={handleChangeDate}
+								name="daysUntilNextPurchase"
+								type="radio"
+								id="soon"


added id attribute to match forHTML attribute on <label> for accessibility

rachelspencer · 2024-02-15T00:18:12Z

src/views/ManageList.jsx

+						<li>
+							<label htmlFor="soon"> Soon (7 days)</label>
+							<input
+								value={7}


Consider making this your default value, to enhance accessibility. This can be achieved by adding another attribute here. If you chose to do this, you'll also need to update the initial state value to be "7".

We initially leave this part of the form unselected to account for accidental form submissions where an item name would be provided but the first radio button would already be selected, before the user has a chance to select it on their own. Whereas if there is no default radio button value, the user would see an error message, asking them to select an option first. See below screenshot.

As an alternative, we added a clear "Please select option" instruction to make it clear that the user must make a selection before submitting the form. See below screenshot.

@trushmi What do you think?

src/views/ManageList.jsx

…e no longer necessary

sillytsundere · 2024-02-15T20:49:20Z

Jose and I made more changes, we agree with the changes suggested and implemented by Rachel to make the items of the form required so as to prevent items with no name or next purchase date to be entered into the list. Also Jose refactored the code to eliminate the need for useState and refactored the multiple hooks into one handleSubmit hook. This also removes the const newItem, an unused variable, calling the await directly without saving it in a variable.

trushmi

Looks good overall, but I think we can make a few improvements.

src/views/ManageList.jsx

trushmi · 2024-02-15T21:48:36Z

src/views/ManageList.jsx

-			Hello from the <code>/manage-list</code> page!
-		</p>
+		<div>
+			<form id="item-form" onSubmit={handleSubmit}>


I would recommend moving this form to a separate component in case we use it in the future. For example, we can create a component called RadioInput. The component should use props to receive the options, like this:

const options = [{label: "Soon (7 days)", value: 7, id: "soon"}, {label: "Kind of soon (14 days)", value: 14, id:"kind-of-soon"}]; ... <RadioInput name="daysUntilNextPurchase" options={options}/>

and inside of the component we can use those props:

function RadioInput({name, options}) { return ( <ul> {options.map((option) => ( <li> <label htmlFor={option.id}>{option.label}</label> <input value={option.value} name={name}... > </li> ) )} <ul/> ) }

This is a good suggestion. However, Paige and I think it's unnecessary for now simply because we don't think a radio button component will be needed in the future, based on the acceptance criteria of future issues. But, we can always keep this suggestion in mind in case it does become a necessary change in the future.

src/views/ManageList.jsx

rachelspencer

@sillytsundere @jaguilar89 looks good!

trushmi

Reviewed and tested. Looks good to me!

timothy-taylor

Nice job completing this one! I left a few thoughts: feel to make any small changes you want or just make note of any changes you want to make moving forward (you can definitely leave the custom validation + status stuff for the future) and get this merged in!

timothy-taylor · 2024-02-16T21:01:31Z

src/views/ManageList.jsx

+	const handleSubmit = async (e) => {
+		e.preventDefault();
+
+		const formData = new FormData(e.target);


Cool approach here using the FormData api in conjunction with default form/input functionality to access necessary data. You don't see this often in client-side React apps, but totally legit thing to do so that's cool to see.

timothy-taylor · 2024-02-16T21:02:56Z

src/views/ManageList.jsx

+			alert('There was a problem');
+		}
+
+		formRef.current.reset();


I wonder whether this reset() should be inside of the try branch of the try ... catch .... If there is an error that means that the item wasn't saved, so we probably don't want to clear the form on people (in case they want to try again?)

This makes sense, clearing a form after a successful submit is expected behavior, not clearing it would clearly indicate there was a problem. I added this change.

timothy-taylor · 2024-02-16T21:04:28Z

src/views/ManageList.jsx

+
+			alert('Item saved successfully');
+		} catch (error) {
+			alert('There was a problem');


We don't necessarily have to fully display the error in this catch statement, but I do think we could make this error status more specific.

I modified the catch block to display a more detailed error message.

try { ... } catch (error) { alert(`There was a problem: ${error.message}`); };

timothy-taylor · 2024-02-16T21:06:34Z

src/views/ManageList.jsx

+							name="daysUntilNextPurchase"
+							type="radio"
+							id="soon"
+							required


I said this to the other group as well, but the built in HTML validations actually aren't as accessible as one would hope. So I think down the road we should build our own validation + status displaying logic.

timothy-taylor · 2024-02-16T21:11:14Z

src/views/ManageList.jsx

+import { useRef } from 'react';
+
+export function ManageList({ listPath }) {
+	const formRef = useRef();


the best practice recommendation by the React team is to declare this as const formRef = useRef(null) if you are going to assign the reference to a DOM element. I'm honestly not sure the different null vs undefined makes in the initial declaration in reality, but I would still recommend doing it so that your code can be as consistent as possible.

Noted. I will add this change. Thanks!

…e for useRef, moved form reset into try block so it only resets upon successful submission

jaguilar89 and others added 3 commits February 12, 2024 15:59

edit ManageList.jsx

f49ce05

finished form on manage list view, refactored addItem function to sav…

0f24cac

…e item to database on list

removed comment

e32236d

sillytsundere added the enhancement New feature or request label Feb 12, 2024

sillytsundere assigned trushmi and rachelspencer Feb 12, 2024

enhance accessibility and fixform submission requirements

854f7b8

rachelspencer approved these changes Feb 15, 2024

View reviewed changes

refactored code, simplified hooks into one handleSubmit hook, useStat…

667147e

…e no longer necessary

sillytsundere assigned jaguilar89 and sillytsundere and unassigned trushmi and rachelspencer Feb 15, 2024

sillytsundere requested review from trushmi and rachelspencer February 15, 2024 20:52

trushmi requested changes Feb 16, 2024

View reviewed changes

rachelspencer approved these changes Feb 16, 2024

View reviewed changes

made suggested changes to ManageList.jsx

4160494

trushmi approved these changes Feb 16, 2024

View reviewed changes

trushmi requested a review from timothy-taylor February 16, 2024 19:57

timothy-taylor approved these changes Feb 16, 2024

View reviewed changes

added suggested changes to ManageList.jsx; made null the initial valu…

078f475

…e for useRef, moved form reset into try block so it only resets upon successful submission

sillytsundere merged commit 98a596b into main Feb 16, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 5. Add Items to list #20

Issue 5. Add Items to list #20

sillytsundere commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading

rachelspencer commented Feb 13, 2024 •

edited

Loading

rachelspencer left a comment

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

rachelspencer Feb 15, 2024

jaguilar89 Feb 16, 2024 •

edited

Loading

jaguilar89 Feb 16, 2024

sillytsundere commented Feb 15, 2024

trushmi left a comment

trushmi Feb 15, 2024

jaguilar89 Feb 16, 2024 •

edited

Loading

rachelspencer left a comment

trushmi left a comment

timothy-taylor left a comment

timothy-taylor Feb 16, 2024

timothy-taylor Feb 16, 2024

jaguilar89 Feb 16, 2024 •

edited

Loading

timothy-taylor Feb 16, 2024

jaguilar89 Feb 16, 2024

timothy-taylor Feb 16, 2024

timothy-taylor Feb 16, 2024

jaguilar89 Feb 16, 2024

Issue 5. Add Items to list #20

Issue 5. Add Items to list #20

Conversation

sillytsundere commented Feb 12, 2024 • edited Loading

Description

Related Issue

Acceptance Criteria

Type of Changes

Updates

Before

After

Testing Steps / QA Criteria

github-actions bot commented Feb 12, 2024 • edited Loading

rachelspencer commented Feb 13, 2024 • edited Loading

rachelspencer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaguilar89 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sillytsundere commented Feb 15, 2024

trushmi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaguilar89 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

rachelspencer left a comment

Choose a reason for hiding this comment

trushmi left a comment

Choose a reason for hiding this comment

timothy-taylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaguilar89 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sillytsundere commented Feb 12, 2024 •

edited

Loading

github-actions bot commented Feb 12, 2024 •

edited

Loading

rachelspencer commented Feb 13, 2024 •

edited

Loading

jaguilar89 Feb 16, 2024 •

edited

Loading

jaguilar89 Feb 16, 2024 •

edited

Loading

jaguilar89 Feb 16, 2024 •

edited

Loading