Skip to content
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

Onboarding Contextual Tutorial Rendering #3063

Conversation

alessandroboron
Copy link
Contributor

@alessandroboron alessandroboron commented Jul 10, 2024

Task/Issue URL: https://app.asana.com/0/72649045549333/1207701578629995/f
CC: @SabrinaTardio

Description:

This PR changes the rendering of the onboarding contextual messages based on the cohorts.

For cohort ma show the Old onboarding overlays
For cohort mb show the new Dax Contextual Dialogs.

I created a ContextualOnboardingPresenter and a ContextualDaxDialogsFactory.

The ContextualDaxDialogsFactory responsibility is to create specific contextual dialogs based on the BrowsingSpec. In this PR, only one type is created. In the next PRs different types will be returned.

The presenter is in charge of:

  • Perform the DaxDialog segue and show the messages as full-screen overlay if the cohort is ma
  • Ask the ContextualDaxDialogsFactory for the correct dialog, add it as a child and render it as part of the Tab screen.

Video

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-10.at.19.28.02.mp4

Steps to test this PR:

SCENARIO 1 - Old Onboarding
Prerequisites: Delete DuckDuckGo App from Simulator.

  1. DuckDuckGo Scheme -> Run -> Arguments -> Environment Variables -> VARIANT -> ma
  2. Add temporary newInstallCompletion(self) at line 141 in DefaultVariantManager
  3. Launch the App.
  4. Complete the Onboarding Intro.
  5. The New Tab Page should show the old Dax Dialog.
  6. Follow the instructions and input a website (e.g. “facebook.com”) into the address bar.
    Expected Result: An overlay informing the user about the trackers blocked should appear on top of the screen.
  7. Dismiss the overlay.
  8. Tap the fire Button.
    Expected Result: The action sheet to clear the data should appear on the screen along with an overlay informing the user about the fire button.

SCENARIO 2 - New Onboarding
Prerequisites: Delete DuckDuckGo App from Simulator.

  1. DuckDuckGo Scheme -> Run -> Arguments -> Environment Variables -> VARIANT -> mb
  2. Add temporary newInstallCompletion(self) at line 141 in DefaultVariantManager
  3. Launch the App
  4. Complete the Onboarding Intro.
  5. The New Tab Page should show the old Dax Dialog.
  6. Follow the instructions and input a website (e.g. “facebook.com”) into the address bar.
    Expected Result: The Web page should shift down and the Dax Contextual Dialog should appear on the screen.
  7. Tap the action button of the Dax Contextual Dialog.
    Expected Result: The Dax Contextual Dialog should disappear and the web page shift up.
  8. Tap the fire Button.
    Expected Result: The action sheet to clear the data should appear on screen but it should not show any overlay.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

Copy link

🚫 The Asana task linked in the PR description is not added to iOS App Board project.

  1. Verify that the correct task is linked in the PR.
    • ⚠️ Please use the actual implementation task, rather than the Code Review subtask.
  2. Verify that the task is added to iOS App Board project.
  3. When ready, remove the bot: not in app board label to retrigger the check.

@github-actions github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to iOS App Board Asana project label Jul 10, 2024
@@ -616,11 +616,13 @@
9F23B8032C2BCD0000950875 /* DaxDialogStyles.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9F23B8022C2BCD0000950875 /* DaxDialogStyles.swift */; };
9F23B8062C2BE22700950875 /* OnboardingIntroViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9F23B8052C2BE22700950875 /* OnboardingIntroViewModelTests.swift */; };
9F23B8092C2BE9B700950875 /* MockURLOpener.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9F23B8082C2BE9B700950875 /* MockURLOpener.swift */; };
9F2510142BF5809E0096DB16 /* SubscriptionFlowViewModelTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9F2510132BF5809E0096DB16 /* SubscriptionFlowViewModelTests.swift */; };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were Recovered References due to a merge conflict issue. I checked the code and SubscriptionFlowViewModelTests.swift and SubscriptionContainerViewModelTests.swift are still part of the project.

@@ -28,11 +28,15 @@
<rect key="frame" x="0.0" y="0.0" width="768" height="1024"/>
<autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/>
<subviews>
Copy link
Contributor Author

@alessandroboron alessandroboron Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To simplify the review:

  • I’ve added a vertical stack view and pinned its edges to the superview.
  • I moved the webViewContainer within the stackview.

}
}
controller.view.isHidden = true
controller.view.alpha = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIR I hadn't set the alpha to animate the appearance/disappearance of a view in a stack view. However, without it, the view simply appears without any animation. The disappearance works fine without the alpha 😕


// MARK: - TabViewControllerType

protocol TabViewControllerType: UIViewController {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing the presenter

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works as advertised! :)

However, in the onboarding flow on iPad mini in landscape Dax is cut off on the welcome view and the bottom of the dax dialog is lost on the browser comparison.

I've asked on the ship review for Figma updates showing different screen sizes and orientations so that we know what scale to expect, but if you don't get any feedback by the time you're back on it, please just do your best to get it looking reasonable :)

@alessandroboron alessandroboron force-pushed the alessandro/onboarding-tutorial-show-dax branch from 6b60864 to 4391b90 Compare July 16, 2024 03:37
@alessandroboron alessandroboron changed the base branch from AleSab/implement-contextual-onboarding to sabrina/new-tab-onboarding July 16, 2024 03:37
Base automatically changed from sabrina/new-tab-onboarding to AleSab/implement-contextual-onboarding July 16, 2024 17:55
@alessandroboron alessandroboron force-pushed the alessandro/onboarding-tutorial-show-dax branch from 4391b90 to b7e6942 Compare July 17, 2024 00:37
@alessandroboron alessandroboron requested a review from brindy July 17, 2024 00:37
@alessandroboron
Copy link
Contributor Author

This works as advertised! :)

However, in the onboarding flow on iPad mini in landscape Dax is cut off on the welcome view and the bottom of the dax dialog is lost on the browser comparison.

I've asked on the ship review for Figma updates showing different screen sizes and orientations so that we know what scale to expect, but if you don't get any feedback by the time you're back on it, please just do your best to get it looking reasonable :)

This is now rebased on top of main contextual feature branch and contains the fixes for iPad mini in landscape

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alessandroboron alessandroboron force-pushed the alessandro/onboarding-tutorial-show-dax branch from b7e6942 to 5709cf7 Compare July 18, 2024 21:38
@alessandroboron alessandroboron merged commit 580256d into AleSab/implement-contextual-onboarding Jul 18, 2024
13 of 14 checks passed
@alessandroboron alessandroboron deleted the alessandro/onboarding-tutorial-show-dax branch July 18, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: not in app board Added by automation for pull requests with tasks not added to iOS App Board Asana project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants