Skip to content
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

He rb new list setup #47

Merged
merged 7 commits into from
Feb 11, 2020
Merged

He rb new list setup #47

merged 7 commits into from
Feb 11, 2020

Conversation

RachaelSkye
Copy link
Contributor

Just reposting the comment I made on the issue:

The file structure got somewhat re-organized. After the changes some components weren't being used so they were removed.

There were firebase errors because we weren't using the firebase initialization function in the firebase.js file. Every firebase call uses it now.

We're also confirming success and catching errors on adding items in the form component. As a user I get a little confused if I don't see some kind of feedback when submitting a form.

The items list component got refactored because using component didmount/update in the class based component was causing a memory leak error. It's functional now and using hooks.

Now a user can enter a pre-existing token on the home page or generate a new one.

Navigating to add items will allow a user to add items to the collection associated with the token or create a new collection with the newly generated token. A user will be able to see added items or any items already in the db for the entered token on the home page.

haleyelder and others added 6 commits February 3, 2020 20:38
…e and to app state. Altered componentDidMount in Item.js to query userThree and map function that displays list to reflect userThree item names.
…own the token and the function to generate a token as props.
…itialization, refactored ItemsList to be functional to use hooks.
Copy link
Contributor

@jcwesley93 jcwesley93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me! I like the new file structure and I am glad you both were able to figure out the firebase importing. The code looks clean and readable. Good job!

src/Form/Form.js Show resolved Hide resolved
Copy link

@rmorabia rmorabia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about the implementation -- As far as I can tell from the ticket, the implementation calls for us to call the token from localStorage and check that against Firebase to call an existing list, not to let the user manually submit the user token.

Confirmation @segdeha & @stacietaylorcima?

Copy link
Contributor

@msholty-fd msholty-fd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left one comment about default exports that you can take on if you want. Nice work! 💪

src/List/ItemList.js Show resolved Hide resolved
src/lib/firebase.js Outdated Show resolved Hide resolved
src/List/ItemList.js Show resolved Hide resolved
@segdeha
Copy link
Member

segdeha commented Feb 9, 2020

I'm a bit confused about the implementation -- As far as I can tell from the ticket, the implementation calls for us to call the token from localStorage and check that against Firebase to call an existing list, not to let the user manually submit the user token.

Confirmation @segdeha & @stacietaylorcima?

That's correct. Allowing the user to input an existing token is covered in story #27 (5). We might have some overachievers on our hands 😅

@RachaelSkye RachaelSkye merged commit d1e428a into master Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants