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 Intro Main Screens #2994

Conversation

alessandroboron
Copy link
Contributor

@alessandroboron alessandroboron commented Jun 26, 2024

Task/Issue URL: https://app.asana.com/0/1206329551987282/1207569383033362/f
CC: @SabrinaTardio

Description:

This PR adds the Onboarding Intro screens.

Steps to test this PR:

  1. Use OnboardingView Live SwiftUI Preview and go through the intro flow steps.
  2. Repeat 1 using:
    2.1 iPad
    2.2 iPhone SE
    2.3 Light - Dark mode

NOTE
I will add the OnboardingIntroViewController and Menu Debug option to present the flow in another PR.

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 14
  • iOS 15
  • iOS 16

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

Copy link

github-actions bot commented Jun 26, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 95e9832

import SwiftUI
import class UIKit.UIScreen

final class MetricBuilder<T> {
Copy link
Contributor Author

@alessandroboron alessandroboron Jun 26, 2024

Choose a reason for hiding this comment

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

The idea behind this class is to build device-specific values for the UI. For example, a scenario where the top padding for the iPhone is 100 and the iPad is 200.

This class could be extended with additional helpers such as:

func value(iPhonePortrait: T, iPhoneLandscape: T) -> Self and so on, but I hope the design won't change much between devices and we won’t need them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, that's nice. We have "AppWidthObserver" in the rest of the app. Wonder if that can be refactored to use this somehow in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, indeed, I was thinking that I could write some tests and move this to Core or a local package so we could reuse it across the App.

}
}

private var lightGradient: some View {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we put these colours in the asset catalogue?

Copy link
Contributor

@brindy brindy Jun 26, 2024

Choose a reason for hiding this comment

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

Nah, these seem really specific to the onboarding and we'd just be generating a bunch of assets to avoid having to use the colorScheme. It's nicely self contained with a specific reason d'être so this seems like a reasonable exception to the rule imo.

@alessandroboron alessandroboron requested a review from brindy June 26, 2024 10:58
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! Really nice work. Can't wait to see that in the app for real.

Base automatically changed from alessandro/onboarding-intro-dax-browser-comparison to alessandro/onboarding-intro June 27, 2024 00:12
@alessandroboron alessandroboron force-pushed the alessandro/onboarding-intro-main-screen branch from 3b1b954 to 95e9832 Compare June 27, 2024 00:39
@alessandroboron alessandroboron merged commit 94b83ff into alessandro/onboarding-intro Jun 27, 2024
14 checks passed
@alessandroboron alessandroboron deleted the alessandro/onboarding-intro-main-screen branch June 27, 2024 00:57
alessandroboron added a commit that referenced this pull request Jul 2, 2024
alessandroboron added a commit that referenced this pull request Jul 8, 2024
alessandroboron added a commit that referenced this pull request Jul 12, 2024
alessandroboron added a commit that referenced this pull request Jul 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants