Skip to content

ionic-firebase integration - #1

Open
Hogusong wants to merge 27 commits into
masterfrom
dev-young
Open

ionic-firebase integration#1
Hogusong wants to merge 27 commits into
masterfrom
dev-young

Conversation

@Hogusong

Copy link
Copy Markdown

No description provided.

“Young and others added 22 commits December 10, 2018 11:02
@Hogusong
Hogusong requested a review from jedihacks December 28, 2018 19:11
@Hogusong Hogusong changed the title Dev young ionic-firebase integration Dec 28, 2018
@Hogusong
Hogusong requested a review from Fdom92 January 3, 2019 18:37
@luiskcs89

Copy link
Copy Markdown

I still haven't been able to run the project again, but I took a look at the code it looks good, here are some suggestions for the dev process:

  • To avoid these packages conflict try to not change that many npm packages all at once in the package json. If you do, delete your node_modules and run npm install again and run the app to make sure it still works, because you may be using old packages and when someone else clones your branch it might not work.
  • Make smaller PRs. You can have a main branch, we usually use develop, and in a project like this you could make dev-young your main branch and create smaller branches that have small pieces of functionality, then all these PRs can be checked separately, quickly and merged into the main branch.
  • Add a small description about what your PR does. Usually, the code reviewer will not know what to test for and without a description he/she might overlook some of the functionality you implemented.
  • You can use something like commitizen https://github.com/commitizen/cz-cli to make the commits more organized and to give them some context of wether they are a new feature, a fix, a refactor, etc.
  • Update the readme with any information that others need in order to run. (For example on where to find the environment.ts or how to set it up with the their own firebase).
    Other than that, even without running the app, the code looks clean and looks like it does what it is supposed to, so good job!

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.

2 participants