deg / re-frame-firebase

Re-frame wrapper around Google's Firebase database
95 stars 32 forks source link

Added email authentication/registration #16

Closed wneirynck closed 6 years ago

wneirynck commented 6 years ago

Email (password) auth is not the same as oauth, so I had to roll my own functions. Didn't find any tests, so I didn't bother writing any. It would be very difficult to unit-test anyway.

deg commented 6 years ago

Thanks! This code looks straightforward and good.

I'd just like two small changes:

1) Please add some documentation to README.md. It should go in the Authentication section and (like for oAuth) should list the email provider and explain how to set it up in Firebase.

2) I'm a bit pedantic about extra whitespace at the end of lines, especially when since it causes GitHub's diff to report irrelevant changes. Can you remove the whitespace at the end of lines 105-108.

And, if you have some extra time, it would be fantastic if you could contribute some code, or a small project that supplies a working example of using email authentication. (But, this is just a nice-to-have. I'll certainly accept this PR without it).

wneirynck commented 6 years ago

I agree, it occurred to me last night that I forgot to update the documentation. See my latest updates. As for the whitespaces, there weren't any at the end of the lines, but I inserted one in the middle so they would align more nicely. But since I have added some code comments in between, I have removed the space again.

I guess I could strip down my project (it's a small one) where I needed this authentication strategy so it can be used as an example, I'll see what I can do.

wneirynck commented 6 years ago

Ok, changed it

deg commented 6 years ago

Thanks!!