BIT-155: Adds the landing screen and Auth module #20
Conversation
nathan-livefront
requested review from
fedemkr,
jubie-livefront and
matt-livefront
|
No New Or Fixed Issues Found |
Code coverageTotal coverage:
|
| File | Coverage |
|---|---|
| GlobalTestHelpers/MockAppModule.swift | 0.00% |
| GlobalTestHelpers/MockAppModule.swift | 0.00% |
| GlobalTestHelpers/MockAppModule.swift | 0.00% |
Powered by Slather
Generated by 🚫 Danger
| override func setUp() { | ||
| super.setUp() | ||
| rootNavigator = MockRootNavigator() | ||
| stackNavigator = MockStackNavigator() | ||
| subject = AuthCoordinator( | ||
| rootNavigator: rootNavigator, | ||
| stackNavigator: stackNavigator | ||
| ) | ||
| } |
There was a problem hiding this comment.
🤔 Should tests be added with rootNavigator being nil? Not sure which would be the real case where the AuthCoordinator receives a nil root value but if I'm not mistaken it's currently feasible.
There was a problem hiding this comment.
Good catch! That is something that is currently feasible, so I'll add tests for that.
There was a problem hiding this comment.
On second thought, I'm going to change rootNavigator to not be optional. I can't think of a real-world use case for it being nil, since the auth flow will always need to be displayed within another context. So I'll update this property and not add any tests!
Scratch all this 🤦♂️ rootNavigator is optional because the variable is weak, I've added a test that validates that AuthCoordinator does not retain a strong reference to rootNavigator!
There was a problem hiding this comment.
oh that makes perfect sense, thanks!
There was a problem hiding this comment.
Just one final question @nathan-livefront, is it possible that if the weak reference is lost, the caller can execute func navigate(to route: AuthRoute, context: AnyObject?) and if so, would you foresee any problems? I guess the stackNavigator would be still in memory so the action could be executed but nothing will be seen by the user given that I assume the rootNavigator won't be in screen if the reference has been lost.
Does it makes sense to add tests for when rootNavigator is nil for start and navigate?
There was a problem hiding this comment.
While it is possible for the caller to execute the navigate and start methods on an instance of AuthCoordinator while rootNavigator is nil, I don't think it's necessarily worth testing in this case.
rootNavigator is weak because the rootNavigator is a property of the parent that is displaying AuthCoordinator. We want this property to be weak so that AuthCoordinator does not strongly hold a reference to its parent (and thus create a circular reference/memory leak).
The only time that rootNavigator would be nil is if AuthCoordinator's parent was no longer being displayed, but a reference to AuthCoordinator was still retained somewhere. In that case, you are totally correct. Both the start and navigate methods could technically be called on AuthCoordinator, but they would have no visible effect if AuthCoordinator's stackNavigator is not being displayed.
🎟️ Tracking
BIT-155
🚧 Type of change
📔 Objective
This PR adds a draft of the Landing screen. The UI for this screen is not final, and is only being added in its current state so that we can continue building out the app while the design team is finishing up the new designs. This draft was built with basic SwiftUI components that we can replace or style in a later PR.
In addition to adding the screen, I also added the scaffolding for navigating through the auth flow. Tapping any of the buttons on the landing screen will take the user to example screens that will be replaced in future PRs.
Note, the Bitwarden logo is missing from this screen, since the asset for the logo was added in #19. The logo will be added to this screen when the UI is updated to match the new designs.
📋 Code changes
.onboardingroute to.auth(AuthRoute)to support navigating to specific screens in the auth flow.📸 Screenshots
RocketSim_Recording_iPhone_14_Pro_2023-09-05_16.52.26.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes