feat: add phone number authentication#110
feat: add phone number authentication#110lkmandy wants to merge 1 commit intoFlutterPlaza:developmentfrom
Conversation
kefeh
left a comment
kefeh
left a comment
There was a problem hiding this comment.
This was a great start, I like the structuring and the implementations details, kudus. I left a few comments, hope it helps
...th_phone_number/authentication_with_phone_number_bloc/phone_number_authentication_state.dart
Outdated
Show resolved
Hide resolved
| Future<Either<AuthFailure, Unit>> verifyOTPCode({ | ||
| required String phoneNumber, | ||
| required Duration timeOut, | ||
| required PhoneVerificationFailed phoneVerificationFailed, |
There was a problem hiding this comment.
There are three problems i have with this.
- A function should have a maximum of 3 parameters and others can be compiled into a unit either map or class for proper readability etc.
- Avoid passing callbacks as parameters if you if you can.
- Most importantly, you are tightly coupling this facad with Firebase by depending on the firebase specific accessory functions and it defeats the purpose of a facad, you should be able to use even SuperBase and extend this facad without an issue
| required PhoneCodeAutoRetrievalTimeout autoRetrievalTimeout, | ||
| }) async { | ||
| try { | ||
| _firebaseAuth.verifyPhoneNumber( |
There was a problem hiding this comment.
What you can do here is define handlers for PhoneVerificationFailed, PhoneVerificationCompleted, PhoneCodeSent, PhoneCodeAutoRetrievalTimeout as accessory and private functions inside this Repository class and then call them here. If you realize, you see that verificationId is set and used only in this repository, so one thing you can do here is set is as a class variable initialized with an empty string, set the value in PhoneCodeSent and then use it inside verifyAndLogin
|
|
||
| abstract class IPhoneNumberRepositoryFacade { | ||
| Future<Either<AuthFailure, bool>> verifyAndLogin({ | ||
| required String verificationId, |
There was a problem hiding this comment.
same here, remove verificationId so we dont have coupling with firebase to the facad
lkmandy
force-pushed
the
implement-phone-number-authentication
branch
3 times, most recently
from
fa00abb to
b050edb
Compare
lkmandy
force-pushed
the
implement-phone-number-authentication
branch
from
b050edb to
07febba
Compare
- Configure native android files - Implement the logic with flutterbloc
lkmandy
force-pushed
the
implement-phone-number-authentication
branch
from
07febba to
a45590f
Compare
2 participants
Description
Implement phone number authentication using firebase and flutter bloc
Type of Change
Pre-launch Checklist
pubspec.yamlwith an appropriate new version according to the pub versioning philosophy,///).