-
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
Feat: Allowing user to add items to shopping list #22
Conversation
Visit the preview URL for this PR (updated for commit 1792e84): https://tcl-77-smart-shopping-list--pr22-bb-rc-add-items-to-l-6oy33ckf.web.app (expires Sun, 01 Sep 2024 16:57:18 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b77df24030dca7d8b6561cb24957d2273c5e9d72 |
Co-authored-by: Ross Clettenberg <[email protected]>
Co-authored-by: Ross Clettenberg <[email protected]>
…List Co-authored-by: Ross Clettenberg <[email protected]>
Yay! It was great pair programming yesterday! The next step is passing that props object from managelist to firebase!!! |
Yes. I like the idea of React Forms. That is what I was having trouble
finding last night! Some sort of React library for forms would be helpful.
…On Tue, Aug 20, 2024, 3:28 PM Brianna ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/views/ManageList.jsx
<#22 (comment)>
:
> export function ManageList() {
+ const notify = () => toast.success('Item added to list!');
+
+ const handleSubmit = (e) => {
+ e.preventDefault();
⬇️ Suggested change
- e.preventDefault();
+e.preventDefault();
+const itemInfoForm = new FormData(e.target)
+
+console.log({itemName: itemInfoForm.get("item"), daysUntilNextPurchase: itemInfoForm.get("when-to-buy")}
—
Reply to this email directly, view it on GitHub
<#22 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXQW6TKAHLE47GQGJ4JG2GDZSORHJAVCNFSM6AAAAABMYXXYJ2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBZGAZTKOBRG4>
.
You are receiving this because you were mentioned.Message ID:
<the-collab-lab/tcl-77-smart-shopping-list/pull/22/review/2249035817@
github.com>
|
Confirmed in a chat: we don't need any special React Forms libraries or hooks. What we have is fine. If we do anything special, it'll come back to haunt all of us later. My words. |
Co-authored-by: Ross Clettenberg <[email protected]>
…olling the state of form info
… toast to use toast.promise Co-authored-by: Ross Clettenberg <[email protected]>
Left behind some thoughts, but this LGTM! 🚀 Great work! |
Co-authored-by: Ross Clettenberg <[email protected]>
…election, removing redundant console logs, and fixing single quotes in ManageList.jsx Co-authored-by: Brianna <[email protected]>
…s key value pairs. This way the variable numbers can be changed at the top in the purchaseTimelines object. Co-authored-by: Brianna <[email protected]>
Thanks everybody for the sweet feedback! This is so cool! We were able to remove redundant console logs, use key-value pairs in an object for a "purchaseTimeline". We were also able to apply some aria-labels! We tried to change single quotes to double quotes, but it kept reverting to single quotes on some! :D |
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.
The code is so well structured and readable! I saw some really great suggestions that take future implementations into account! Possible interesting take could also have been to dynamically render the input elements through mapping a revised purchaseTimeLines object! In this case, there's only three options, so it's not necessary at all but if the project expanded, it could be a useful code revision to increase scalability and reduce lines of code!
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.
The code is so well structured and readable! I saw some really great suggestions that take future implementations into account! Possible interesting take could also have been to dynamically render the input elements through mapping a revised purchaseTimeLines object! In this case, there's only three options, so it's not necessary at all but if the project expanded, it could be a useful code revision to increase scalability and reduce lines of code!
I think this is something I would definetly like to look at trying in a refactor or revision, like you mention I think it would bring some good things to the code! One of Tanner's comments made me think about doing an array as well! I know it isn't neccary for this PR and maybe not for the project but I'd like to give it a try maybe on a later week because at this moment I'm not sure how to do the refactor for it but I think it would be really nice to get the radio button into a component and use the array to may over and dynamically make them.
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 Brianna and Ross, I know this one was a deeper introduction to firebase as well for you two. The form functions amazing and love seeing you utilize react-hot-toast to fit the alert needs of your form. One thing we may want to consider in the future is the case in which the form submission doesn't work / the addItem call fails. In the image below is what I see when I test with turning off my wifi (such a in an event a user's wifi goes out) and hitting submit for the new item. There the notification pop-up stalls. Just some things to consider later on if you wanted to tackle that use case. Still great work!
For an example of how to fill this template out, see this Pull Request.
Description
This code added the ability for the user add an item to their list. A form was added to the manage list page of the app that has a text input and 3 radio options that are required to submit the form. The listPath prop of the app.js was passed to the manage list component to properly pass the information to the component and allow the required information for the add item call of the database.
Related Issue
Closes #5
Acceptance Criteria
UI-related tasks:
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:label
element associated with itEnter
keyData-related tasks:
console.log
in theaddItem
function insrc/api/firebase.js
is replaced with a function that adds the new document to the Firestore database. That function will be imported from thefirebase/firestore
module.nextPurchasedDate
Type of Changes
enhancement
Updates
Before
Screen.Recording.2024-08-22.at.1.56.13.AM.mov
After
Screen.Recording.2024-08-22.at.3.10.09.AM.mov
Testing Steps / QA Criteria
git pull origin bb-rc-add-items-to-list
and check that branch out with git checkoutbb-rc-add-items-to-list
npm ci
to install the newly added dependencies locally andnpm start
to launch the app.Manage List
page.