-
Notifications
You must be signed in to change notification settings - Fork 3
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
Nk tr create tokenized list #24
Conversation
…ab/tcl-7-smart-shopping-list into NK-TR-create-tokenized-list
…collab-lab/tcl-7-smart-shopping-list into NK-TR-create-tokenized-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.
I was able to cross off all of the acceptance criteria. Thank you for the detailed QA process.
Since we've all been testing different iterations of functionality, it was great to let us know how to clear the local storage that our browsers were already collecting so that we could see the user token functionality in action.
I was able to create a new list, see the local storage token in the console, add an item, and was sent to the the grocery list page and saw the item I had entered.
Using the testing criteria:
new local storage token was successful
I was able to see the log of the cucumber item stored with my token named collection in the 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.
@Tanzi11 @nabilkaz this is awesome, what a great PR 👏 👏 👏. Superb description and QA steps, love it <3!
I had issues testing the whole cycle out in prod as you can see in this video, https://www.loom.com/share/179fbce2c1b74470a14273b5a4e24e80 I'm not really sure why it doesn't work once I create the token. But works fine the moment I reload the page!
I couldn't test this locally bc I'm having internet problems and pulling the branch was taking literally FOREVER 😩.
Still, I think this is 👍 to go, we can figure out the redirect bug later!
One thing that I'd like to get removed before merging is the extra dependencies you added, Mobx and the Firesbase wrapper.
Thanks again for the amazing work!
Update
Welp, I couldn't reproduce this locally...I wonder what the issue is 🤔. I'll try to take a look at it tomorrow morning before our meeting 👍
|
||
/* Functional styles */ | ||
|
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.
Oh interesting approach! Are you linking this better than regular css?
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 am! Technically this is still regular css. Being able to describe what you want using classes is great. Combining it with react components to give you both flexibility and abstraction.
Honestly, just needed to choose a scalable approach and chose this way of organizing the css. It was fun.
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 is very similar to how Tailwind CSS works. I think it would be a really interesting side-quest to explore bringing it into this project if you wanted to go down the "utility styles as classnames" approach, to keep it consistent with an industry standard. For a small project, it's fine to make a few small utility functions, but over time this file will become really big (every time you need a new CSS state) and could be hard to explore without documentation.
@@ -16,7 +17,7 @@ class AddItem extends React.Component { | |||
addItem = e => { | |||
e.preventDefault(); | |||
const db = fb.firestore(); | |||
db.collection('items').add({ | |||
db.collection(getLocalToken()).add({ |
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.
👏 👏 👏 👏
<FirestoreDocument | ||
path="items/tIA3kp24eSWYTNKxA3wl" | ||
<FirestoreCollection | ||
path={`${getLocalToken()}/`} |
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 work :)
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.
Wow, the description and documentation in this PR is 🏅 💯 . I love it!
I tested this out and probably ran into the same issue that @alejandronanez ran into, where when I created my first list it redirected me to /add-item
(full page reload instead of a react-dom
app redirect).
If you try to simply go to https://deploy-preview-24--tcl-7-smart-shopping-list.netlify.app/add-item you'll see the same. I believe this is a server-side configuration that we need to handle in netlify, since all requests should get routed through the root index.html
and not try to access some other document like add-item.html
. @segdeha can you confirm if there is some configuration you've set in the past so this works?
Other than that minor bug, which is not due to this PR or the project code at all, this amazing PR is ready to merge. 👏 👏 👍
@@ -3,15 +3,15 @@ | |||
"version": "0.1.0", | |||
"private": true, | |||
"dependencies": { | |||
"firebase": "^7.14.2", | |||
"firebase": "^7.14.3", |
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.
One thing of note for dependency management - It's generally a good idea to indicate in your PR why you are changing the version of a dependency. This project is super young, so this shouldn't affect anything. However in any long-lived project, even a small version bump comes with risk so we should at least understand what the benefit of upgrading is.
I wouldn't worry about changing this back, but just something to note for the future!
|
||
/* Functional styles */ | ||
|
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 is very similar to how Tailwind CSS works. I think it would be a really interesting side-quest to explore bringing it into this project if you wanted to go down the "utility styles as classnames" approach, to keep it consistent with an industry standard. For a small project, it's fine to make a few small utility functions, but over time this file will become really big (every time you need a new CSS state) and could be hard to explore without documentation.
Hey! We’ve just ignored this issue in the past, but you’re more than welcome to try creating a |
Description
The PR adds routing logic at
/
root, a library for handling tokens on the client, aSignIn
page component for creating a list, functionality ingetList.js
andaddItem.js
to get and create lists based on the clients token and functional css styling to styleSignIn
.We accomplished this by breaking the problem down into the following logic:
/
root check if client has token inlocalStorage
SignIn
.create a new list
then add token to theirlocalStorage
and route user to./add-item
What is
localStorage
?We researched
localStorage
via the MDN documentation that allows you to use the Storage object to store data across browser sessions. By using the method setItem(key, value) you can store a key-value pair that will persist across sessions.Here is a video review of using localStorage (unfortunately there's no sound but you actually don't need it. Nabil walks through viewing, setting and retrieving
localStorage
values in the console): https://www.loom.com/share/9870282bd78049599e7a701381d4e525New token functions
We added set, get, and has functions to
token.js
. We used the naming convention[type]LocalToken
because it denotes both where it islocalStorage
and what it istoken
. It may have been better to useClient
which isn't dependent on how we did it (if we stored it differently on the client).setLocalToken
- stores aString
value generated bygetToken
and stores it in thelocalStorage
under the keytoken
getLocalToken
- retrieves thestring
value of the token stored on the clienthasLocalToken
- returns aboolean
depending on whether a token is found on the client or not.Token-based routing logic at root
/
We use token-based authentication to grant list access to users and have it persist through multiple browser sessions. This is accomplished by generating a token (using
getToken
found in./lib/token.js
) and setting it in thelocalStorage
of the Client. We then use this reference to perform several tasks:localStorage
to create a shopping list when a new item is made and to retrieve collections.signin
orlist-view
page is based on whether a token is found in the ClientslocalStorage
.Creating and Reading collections
Our application now has the CR of CRUD (Create, Read, Update, Delete).
getList.js
andaddItem.js
to retrieve and create collections in firebase.🔥Learning that took us a while -> Firebase implicitly creates collections. 🔥
We got hung up on how to create a new
collection
. We first assumed that we had to create it by adding a newcollection
via thefb
object but after reading the documentation on data modeling there is a line that explicitly states:Which means if we use a name not represented by a collection in firebase when adding a document it will implicitly create a collection of that name in the database.
CSS styling for sign-in page
We took a functional approach to CSS (basing it off of tachyons https://tachyons.io/). This means instead of describing the object in CSS. IE - building out a card class that has shadow, background color, etc. You build a drop-shadow class and bg-blue class and apply each class to the HTML element to style each part.
Pros: It makes it much easier to get in and style new elements once you understand the class syntax.
Cons: Functional CSS can be very atomic so you don't get the simplicity of using a single card class (but that's what React components are for right?) and you must be familiar with the stylistic syntax.
Here's a video walkthrough of using functional CSS: https://www.loom.com/share/c2f3290a268f4df9b2b3fa6d3e4b20a9
Accessibility updates
Ran the accessibility extension and found two errors:
Gray in the sample app (#999999) doesn't pass the accessibility checker (which requires a 4.5 rating). Replaced with #767676 which has a 4.54 contrast ratio rating.
When tabbing it would tab twice to the button "create a shopping list". This was because we had wrapped the button with an anchor tag so we could use onClick and route to a new path. Instead, we replaced it with an action property that points to the intended path
./add-item
. Now it doesn't double tab to the "create a shopping list" button.Here's the new tabbing behavior. Works great :)
Related Issue
Closes #4
Acceptance Criteria
AC:
Type of Changes
Updates
Before
/
root/add-item
/list-view
After
/
root/add-item
/list-view
Testing Steps / QA Criteria
General setup:
From your terminal, pull down this branch with
git pull origin NK-TR-create-tokenized-list
and check that branch out withgit checkout NK-TR-create-tokenized-list
Then
npm install
to install the newly added dependencies locally andnpm start
to launch the app.In your browser navigate to the localhost server that is indicated in your terminal after
npm start
.Testing routing logic:
/
Testing token storage:
/
create shopping list
buttonconsole
tab!!window.localStorage.getItem('token')
into the console.true
Add an item:
Navigate to localhost to the path
/add-item
Write
cucumber
into the input field and hit submit.open developers tools and select the
console
tabCopy and paste
window.localStorage.getItem('token')
into the console and remember this value.In a new tab or window, open and log into https://console.firebase.google.com/u/0/project/tcl-7-smart-shopping-list/database
Find the value that was returned to you in step 4
If found verify that it has a document with a
itemName
field with the valuecucumber
.If so test passed