-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow user to join an existing shopping list #21
Conversation
Visit the preview URL for this PR (updated for commit dc0a3c7): https://tcl-57-smart-shopping-list--pr21-es-jg-join-existing-1hai28xb.web.app (expires Sat, 29 Apr 2023 16:17:10 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: ad3eb6c34c2ec5986fcc218178df5985eb9c9ffb |
…with an existing collection in firestore
…y of QuerySnapshot
…-shopping-list into es-jg-join-existing-list
- remove setting token to empty string in local storage on failed token validation - remove redundant update to tokenExist state variable on failed token validation
… better flow content.
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.
This looks super great guys! We tested it from the preview link. Works as expected. I even used a token name saved in Beth's browser, and was able to bring up her list on my end! Awesome job!!!
*/ | ||
export async function validateToken(userTokenInput) { | ||
const listCollectionRef = collection(db, userTokenInput); | ||
const listSnapshot = await getDocs(listCollectionRef); |
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.
Curious how line 23 and 24 work together? We looked up the firebase "getDocs" function. Still a little fuzzy on how it all works.
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.
Hey Amy! If I'm understanding things correctly, accessing data from a firestore db is a two-part process. You first get a reference to a collection in the db using the collection
method. I like to think of this like requesting an address. And with that reference, you're then able to actually query the db using getDocs
to take "snapshots" of the db state at a particular instance in time. I like to think of this like a camera taking a picture of the db and capturing its state at an instance in time.
Still hazy with all of this myself though so I'm not sure this is completely right.
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.
That's a great explanation!
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.
great explanation- thank you @jongranados !
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.
Great work, @jongranados and @emilysellers! I definitely think it is worth talking about the accessibility issues with everyone on the team with regards to using the Toastify library. Otherwise, the functionality is working as expected!
@@ -15,6 +15,16 @@ export function streamListItems(listId, handleSuccess) { | |||
return onSnapshot(listCollectionRef, handleSuccess); | |||
} | |||
|
|||
/** | |||
* Check existence of list in Firestore associated with user token input. |
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.
Love the documentation here!
src/views/Home.jsx
Outdated
|
||
export function Home({ setListToken }) { | ||
const [userTokenInput, setUserTokenInput] = React.useState(''); |
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.
Suggesting again - import { useState } from 'react'
as opposed to React.useState
. Not a blocker, but a bit cleaner IMO.
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.
Whoops, forgot about this! Done. Definitely cleaner!
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.
Great work! One optional idea for an optimization before we ship.
export async function validateToken(userTokenInput) { | ||
const listCollectionRef = collection(db, userTokenInput); | ||
const listSnapshot = await getDocs(listCollectionRef); | ||
return !listSnapshot.empty; | ||
} |
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.
This function works exactly as intended, but I have an optimization mission, should you choose to accept it.
Before we get to the question "does this list exist on the server?", we know our tokens have to be two things:
- non-empty, and
- a specific shape
Can we use this knowledge to check for validity and return early without wasting time and resources on a request to our database?
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.
Agh, such a great point EJ! I'll incorporate that now.
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.
Actually, before doing so I want to ask a question at today's sync. Going to merge to main for now.
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.
Great point, EJ! It would be great to talk this over today.
<button type="button" onClick={handleClick}> | ||
Create New List | ||
Create new 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.
Yesssss! I'm such a fan of removing unnecessary title case from our UIs!
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.
Excellent job on this issue. I especially liked the explicit and complete pr, and the step by step instructions on how to test, and what you considered re: toastify and there may need to be discussion regarding tradeoffs or approach. Fantastic!
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.
Great work @emilysellers and @jongranados ! Everything's working :) Awesome job!
Description
This PR allows users to join existing shopping list so they can manage them with friends or family. This functionality takes the following steps:
localStorage
.List
view and shown the existing list.The alert is generated here with Toastify which might present some accessibility concerns. The dev team will likely want to discuss the best route for generating alerts throughout the app.
Related Issue
Closes #5.
Acceptance Criteria
If a user doesn’t already have a token:
Home
view shows a form that allows the user to enter a token to join an existing list, in addition to the button that allows them to create a new list.label
element associated with itsEnter
keylocalStorage
List
view and shown the items on that listIf a user does already have a token:
List
view.Type of Changes
Updates
Before
This screenshot shows the application's home page before this functionality was implemented; there was no way for the user to join an existing shopping list unless an existing list's respective token was already stored in their browser's local storage.
After
This screenshot show the application's home page after this functionality was implemented; a new user is given the option to either create a brand new list or join an existing list with the submission of a valid three word token.
This video demonstrates a user attempting to access an existing list. They first attempt to access a list associated with token
this should fail
. Because such a token does not exist in the database, they are notified of their failed attempt through a pop-up message that reads: "Sorry, this list does not exist." The user then attempts to access a list with tokenmy test list
. Because this token does exist, the user is presented with the grocery items associated with this token.after.mov
Testing Steps / QA Criteria
es-jg-join-existing-list
npm i
npm start
tcl-shopping-list-token
/
)this should fail
should:/
)tcl-shopping-list-token
inside your browser's local storagemy test list
should:/list
)tcl-shopping-list-token