-
Notifications
You must be signed in to change notification settings - Fork 80
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
PIA-1058: App navigation setup #51
Conversation
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.
Great job @kp-laura-sempere! Just a couple of comments to see what you think.
Thank you!
|
||
import Foundation | ||
import SwiftUI | ||
import PIALibrary |
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.
I think since this is presentation, we could avoid leaking framework details like SwiftUI. I'd also try to avoid importing PIALibrary on presentation and domain layers. It's true that we will be removing PIALibrary but the AccountProvider
protocol exposes many different methods that this ViewModel does not care about. I'd suggest to have something in between which will only expose the methods the ViewModel will be using.
What do you think?
// TODO: Update this value from the Vpn OnBoarding installation profile screen | ||
@AppStorage(.kOnboardingVpnProfileInstalled) var onBoardingVpnProfileInstalled = true | ||
|
||
let accountProvider: AccountProvider |
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.
I'd suggest the same as the comment above. To have something that hides all the AccountProvider
methods and it exposes only what the ViewModel needs.
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.
Yes, I was thinking the same. I think we can create a AccountProviderType
protocol that only contains the methods that the presentation layer needs.
Let me try to add it 👍
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.
LGTM! Great job @kp-laura-sempere!
RootContainerView
that is responsible to show the correct flow based on the authentication state:AppRouter
that will be used inside each main flow (Not authenticated and Authenticated flows) inside aNavigationStack
:https://developer.apple.com/documentation/swiftui/navigationstack
https://developer.apple.com/documentation/swiftui/migrating-to-new-navigation-types
NOTE: This PR depends on this other PR from PIALibrary:
pia-foss/mobile-ios-library#24