-
Notifications
You must be signed in to change notification settings - Fork 498
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
Authentication: Verify MSISDN screen #6205
Conversation
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/uDgToZ |
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.
Thanks Ismail, looks great to me! Its amazing to have someone else working on FTUE in parallel 😁
I've added some comments inline.
RiotSwiftUI/Modules/Authentication/VerifyMsisdn/AuthenticationVerifyMsisdnModels.swift
Outdated
Show resolved
Hide resolved
RiotSwiftUI/Modules/Authentication/VerifyMsisdn/AuthenticationVerifyMsisdnModels.swift
Show resolved
Hide resolved
.../Modules/Authentication/VerifyMsisdn/Coordinator/AuthenticationVerifyMsisdnCoordinator.swift
Outdated
Show resolved
Hide resolved
.../Modules/Authentication/VerifyMsisdn/Coordinator/AuthenticationVerifyMsisdnCoordinator.swift
Outdated
Show resolved
Hide resolved
...Modules/Authentication/VerifyMsisdn/Test/Unit/AuthenticationVerifyMsisdnViewModelTests.swift
Outdated
Show resolved
Hide resolved
...Modules/Authentication/VerifyMsisdn/Test/Unit/AuthenticationVerifyMsisdnViewModelTests.swift
Show resolved
Hide resolved
RiotSwiftUI/Modules/Authentication/VerifyMsisdn/View/AuthenticationVerifyMsisdnOTPForm.swift
Outdated
Show resolved
Hide resolved
RiotSwiftUI/Modules/Authentication/VerifyMsisdn/View/AuthenticationVerifyMsisdnScreen.swift
Outdated
Show resolved
Hide resolved
RiotSwiftUI/Modules/Authentication/VerifyMsisdn/View/AuthenticationVerifyMsisdnScreen.swift
Outdated
Show resolved
Hide resolved
.../Modules/Authentication/VerifyMsisdn/Coordinator/AuthenticationVerifyMsisdnCoordinator.swift
Outdated
Show resolved
Hide resolved
switch error { | ||
case RegistrationError.invalidPhoneNumber: | ||
authenticationVerifyMsisdnViewModel.displayError(.invalidPhoneNumber) | ||
default: | ||
authenticationVerifyMsisdnViewModel.displayError(.unknown) | ||
} |
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.
For some reason I'd never twigged you can simply switch on an error like this without doing an as? RegistrationError
🤩
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.
Super! LGTM 🙇♂️
Resolves #6182