-
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
added invite friend form and connect with shareList func #24
Conversation
Visit the preview URL for this PR (updated for commit 2212f27): https://tcl-76-smart-shopping-list--pr24-em-mp-invite-others-dj3il9ca.web.app (expires Sun, 08 Sep 2024 10:31:22 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 512b1a88be8ae05fd3e727b99332819df760271d |
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.
Everything looks good, it looks like I was able share a share-list tester list with all 3 devs, and I am able to see the list in everyone's sharedList collection in firebase. I also initially sent it to myself and all of my lists populated there (didn't notice if they were there previously).
For later, or now if it makes sense to, it could be helpful to include the name of the list, so you know which list you are sending. Also, great idea using localStorage to grab the currently selected list!
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.
note: Mini-typo here from existed to exist
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.
Good catch thank you Sarah! Just fixed the typo
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.
Also, great idea to include the name of the list. I had time to make our alert messages a bit more clear. The messages are now more specific but I think it would be great to add which list we are currently using in the UI somewhere. I'm sure we can implement that at some point.
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.
Cutting it really close, but nice work with this one!
I do like @sar-mko 's idea to include the list name. I think this is a feature we should make note of for when styling the UI.
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.
I agree with Sarah! I also think in the future, or whenever it makes sense, I'd like to know what list I'm currently adding to when I'm adding items. Really cool!
…emplate literals for better clarity.
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.
Looks good, and nice idea of including the list name and the invalid email in the message!
Description
Creating a form to allow the user to invite another person to the current shopping list. If the current user is not valid or the input email is not registered, corresponding alert messages will be displayed. Otherwise, the shopping list will be successfully shared.
Related Issue
closes #6
Acceptance Criteria
Type of Changes
feature
Updates
Before
After
Testing Steps / QA Criteria
Tested by inviting an existing user with a new list, and the list name appeared in the Firebase storage shared list array.