Issue #1 | Connect app to Firebase #44
Conversation
This commit adds `firebase` and `react-firestore` as dependancies so that we can interact with our database. A List component was created where we'll be able to house all the list's logic and display the list items that get added to the database. Inside that List component, we've used `FirestoreCollection` which is a component that allows you to interact with a Firestore collection. Using this component, you can access the collection at a given path and provide sort options, perform queries, and paginate data. This component will setup a listener and update whenever our collection is updated in Firestore.
This commit adds an AddItem component that is a form responsible for sending items up to firestore. When we wrap this AddItem component in the higher-order component (HOC) `withFirestore` we're provided some handy dandy methods that make it easy to send documents up to the database. Check out more documentation and examples of how `withFirestore` is used here: https://www.npmjs.com/package/react-firestore#withfirestore
There was a problem hiding this comment.
Thanks for jumping on this issue @stacietaylorcima! 👏👏👏
Nothing blocking—just a couple comments and questions.
Also, love the PR description
| </a> | ||
| </header> | ||
| </div> | ||
| <main> |
There was a problem hiding this comment.
No more bathtub soup 😆
| const AddItem = ({ firestore }) => { | ||
| const [name, setName] = useState(''); | ||
|
|
||
| // Send the new item to Firebase |
There was a problem hiding this comment.
Are these comments just to bring participants up to speed?
There was a problem hiding this comment.
Yeah. In the past, since many participants are newer to react, we've been really generous with comments so that when they jump in to work on the code, they get a better idea of what each piece does.
| Add Item: | ||
| <input value={name} type="text" onChange={handleChange} /> | ||
| </label> | ||
| <input type="submit" value="Add Item" /> |
There was a problem hiding this comment.
Any reason to use input vs button for submission?
There was a problem hiding this comment.
I was originally using a button by then when I was putzing around in the React docs, I saw their example was using an input and wanted to keep it close to the available docs in case participants wanted to investigate. What would you prefer?
React form docs: https://reactjs.org/docs/forms.html#controlled-components
There was a problem hiding this comment.
I would normally say button, but I get where you're coming from. Mirroring the docs will lead to less confusion 👍
| }; | ||
|
|
||
| return ( | ||
| <form onSubmit={handleSubmit}> |
There was a problem hiding this comment.
Nice accessible form 💯
src/pages/AddItem.js
Outdated
| <form onSubmit={handleSubmit}> | ||
| <label> | ||
| Add Item: | ||
| <input value={name} type="text" onChange={handleChange} /> |
There was a problem hiding this comment.
Super minor, but I usually default to adding placeholder attr.
There was a problem hiding this comment.
I'm into that. Will do!
| path="items" | ||
| // Sort the data | ||
| sort="name" | ||
| // isLoading = is a Boolean that represents the loading status for the firebase query. true until an initial payload from Firestore is received. |
There was a problem hiding this comment.
As earlier, are these comments to be breadcrumbs for the next team picking up the next issue?
There was a problem hiding this comment.
Yep! ❤️
src/pages/List.js
Outdated
| return isLoading ? ( | ||
| <Loading /> | ||
| ) : ( | ||
| <main> |
There was a problem hiding this comment.
If there aren't any styles, do we need this wrapping element? I usually try to keep main as the dominant element of a page, but looking at the markup, we'd essentially have:
<body>
<main>
<form></form>
<section>
<main></main>
</section>
</main>
</body>If we really needed the wrapping element, I think div would be fine as it would merely be for styling. Screen readers would pickup the ul element as something of interest and we can also use tabindex if we want to have greater control regarding keyboard nav. But going off the code as it is, wrapping the ul element seems unnecessary.
There was a problem hiding this comment.
No way. This was just a late-night oversight. Fixing now!
There was a problem hiding this comment.
I hear you 😂
| @@ -1,25 +1,13 @@ | |||
| import React from 'react'; | |||
| import logo from './logo.svg'; | |||
| import './App.css'; | |||
| import List from './pages/List'; | |||
There was a problem hiding this comment.
Bit of a nitpick: I'm not sure I get the folder structure here. We have two modules that are in pages but they both make up the same page. I would expect a Home component to live in pages, and then AddItem and List would be components.
There was a problem hiding this comment.
You're totally right. Per the scope of this issue, the folder structure should be much more like you're mentioning.
I was working ahead and trying to work in the routing code that the other pair wrote but hasn't yet merged. In that PR they are routing to two separate pages, so I set this up so that we could have the List and AddItem components be separate pages.
I think lets leave this for now since it'll help give direction to the merge conflicts today.
There was a problem hiding this comment.
Ah ok, that makes sense 👍
| @@ -0,0 +1,7 @@ | |||
| import React from 'react'; | |||
|
|
|||
| const Loading = () => { | |||
There was a problem hiding this comment.
Reusable loading state ❤️
|
@stacietaylorcima I added cypress to the project with a simple test. It's skipped for now, since we don't have a way of getting a clean test state. The test works and passes, but we have to update Firebase to clear out all the 🥑. Have we been integrating cypress with CI in other cohorts? |
stacietaylorcima
force-pushed
the
stc-connect-firebase
branch
from
cef838e
to
7a6ccdb
Compare
|
We have not been updating Cypress with CI in other cohorts, @stevelikesmusic. ✨ |
Type of Changes
Acceptance Criteria
Related Issue
Closes #23
Description
This PR adds two new components: one for adding new items to the database and another for displaying the list of items.
Display items from database
Inside that
Listcomponent, we've usedFirestoreCollectionwhich is a component that allows you to interact with a Firestore collection. Using this component, you can access the collection at a given path and provide sort options, perform queries, and paginate data.This component will setup a listener and update whenever our collection is updated in Firestore.
More documentation on
FirestoreCollectionhere.Write items to the database
When we wrap this
AddItemcomponent in the higher-order component (HOC)withFirestorewe're provided some handy dandy methods that make it easy to send documents up to the database.Check out more documentation and examples of how
withFirestoreis used here.More documentation on React forms and controlled components here.
Testing Steps / QA Criteria
git pull origin stc-connect-firebaseand check that branch out withgit checkout stc-connect-firebaseyarnto install the newly added dependencies locally andyarn startto launch the app.itemscollection of the database here.TODOs