[MOB-2759] Onboarding card NTP Experiment #778
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,27 @@ class LaunchCoordinator: BaseCoordinator, | |
| func start(with launchType: LaunchType) { | ||
| let isFullScreen = launchType.isFullScreenAvailable(isIphone: isIphone) | ||
| switch launchType { | ||
| /* Ecosia: Change to support `OnboardingCardNTPExperiment` conditions | ||
| case .intro(let manager): | ||
| presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) | ||
| */ | ||
| case .intro(let manager, let checkExperiment): | ||
| guard checkExperiment else { | ||
| presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) | ||
| return | ||
| } | ||
| // TODO: Refactor `FeatureManagement.fetchConfiguration()` pre-condition - maybe a notification from FeatureManagement? | ||
| Task { | ||
| await FeatureManagement.fetchConfiguration() | ||
|
There was a problem hiding this comment. I am wondering, what's seen on screen while this is being executed? Slightly relates to Luca's comment from line 52, but I would expect that at this point we have fetched the Unleash configuration already, so that we can make a synchronous decision. Don't we still have a LoadingScreen that's used when we get an invite code? We could bundle all async start up tasks there. There was a problem hiding this comment. I agree with all the above and YES, it does require a refactor as we are all far from being happy about it 😅 . I tackled next steps already, creating a DRAFT RFC where I'm collecting info regarding the improvement. |
||
| DispatchQueue.main.async { [weak self] in | ||
| guard let self = self else { return } | ||
| guard !OnboardingCardNTPExperiment.isEnabled else { | ||
| self.parentCoordinator?.didFinishLaunch(from: self) | ||
| return | ||
| } | ||
| self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) | ||
| } | ||
| } | ||
|
Comment on lines
+41
to
+52
There was a problem hiding this comment. For now this is working, but I don't think it looks great as it refreshes Unleash one extra time and mixes two concurrency patterns. Maybe this could be some sort of Notification sent by There was a problem hiding this comment. We'll need to think about it thoroughly. For the time being, given the experiment context, I thought about leaving it as is. As a next step, I made a DRAFT RFC in our Confluence page that will be document with a proposed solution for us to comment on and eventually apply. |
||
| case .update(let viewModel): | ||
| presentUpdateOnboarding(with: viewModel, isFullScreen: isFullScreen) | ||
| case .defaultBrowser: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at http://mozilla.org/MPL/2.0/ | ||
|
|
||
| import Foundation | ||
| import Core | ||
|
|
||
| struct OnboardingCardNTPExperiment { | ||
| private enum Variant: String { | ||
| case control | ||
| case first = "test1" | ||
| case second = "test2" | ||
| } | ||
|
|
||
| private init() {} | ||
|
|
||
| static var isEnabled: Bool { | ||
| Unleash.isEnabled(.onboardingCardNTP) && variant != .control | ||
| } | ||
|
|
||
| static private var variant: Variant { | ||
| Variant(rawValue: Unleash.getVariant(.onboardingCardNTP).name) ?? .control | ||
| } | ||
|
|
||
| // MARK: Analytics | ||
| static var analyticsIntroCategory: String? { | ||
| isEnabled ? "intro_card" : nil | ||
| } | ||
|
|
||
| static var analyticsProperty: String? { | ||
| switch variant { | ||
| case .first: | ||
| return "first_copy" | ||
| case .second: | ||
| return "second_copy" | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| /// Send onboarding card view analytics event, but just the first time it's called. | ||
| static func trackExperimentImpression() { | ||
| let trackExperimentImpressionKey = "onboardingCardNTPExperimentImpression" | ||
| guard !UserDefaults.standard.bool(forKey: trackExperimentImpressionKey) else { | ||
| return | ||
| } | ||
| Analytics.shared.ntpOnboardingCardExperiment(.view) | ||
| UserDefaults.standard.setValue(true, forKey: trackExperimentImpressionKey) | ||
| } | ||
|
|
||
| // MARK: Card dismissed | ||
| static private let cardDismissedKey = "onboardingCardNTPExperimentDismissed" | ||
|
|
||
| static var shouldShowCard: Bool { | ||
| isEnabled && !UserDefaults.standard.bool(forKey: cardDismissedKey) | ||
| } | ||
|
|
||
| static func setCardDismissed() { | ||
| UserDefaults.standard.set(true, forKey: cardDismissedKey) | ||
| } | ||
|
|
||
| // MARK: Texts | ||
| static var title: String { | ||
| switch variant { | ||
| case .first: | ||
| return .localized(.onboardingCardNTPExperimentTitle1) | ||
| case .second: | ||
| return .localized(.onboardingCardNTPExperimentTitle2) | ||
| default: | ||
| return "" | ||
| } | ||
| } | ||
|
|
||
| static var description: String { | ||
| switch variant { | ||
| case .first: | ||
| return .localized(.onboardingCardNTPExperimentDescription1) | ||
| case .second: | ||
| return .localized(.onboardingCardNTPExperimentDescription2) | ||
| default: | ||
| return "" | ||
| } | ||
| } | ||
|
|
||
| static var buttonTitle: String { | ||
| switch variant { | ||
| case .first: | ||
| return .localized(.onboardingCardNTPExperimentButtonText1) | ||
| case .second: | ||
| return .localized(.onboardingCardNTPExperimentButtonText2) | ||
| default: | ||
| return "" | ||
| } | ||
| } | ||
| } |
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.
just a minor comment:
skipExperimentalready says what the method should do, no real reason to useforce, or is there? ⚔️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.
The real reason behind it. I guess it was added mostly to advocate the imperative instruction. Can update 👍