-
Notifications
You must be signed in to change notification settings - Fork 27
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
feature/networking #78
Conversation
…tall and npm audit fix.
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 all looks good to me and makes sense. Some random questions.
Before approve, any chance we can fix the lint and build?
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 to me. @tohuynh should get a change to review though.
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.
The NetworkService and PersonService look good to me.
Just have a few comments.
@BrianL3 are you going to try and get the mock firebase into this PR or is this good to go for now? |
Link to Relevant Issue
This pull request resolves the primary blocker for creating containers (e.g. most of the remaining work in the front end): a network layer.
Description of Changes
This is nearly purely additive. It adds a NetworkService which in turn will be used by other Services to request and create their appropriate Model objects. A "demo" of how this might work in included in PersonContainer for now.
Link to Forked Storybook Site
https://Brianl3.github.io/cdp-frontend/