Implements new onboarding UI/UX #1464

diegoreymendez · 2023-08-10T09:19:15Z

Task/Issue URL: https://app.asana.com/0/0/1205257644127197/f

Stack

Feature branch: diego/feature-network-protection-onboarding
- Implements NetP onboarding UserDefaults and debug menu options #1474
  - Implements new onboarding UI/UX #1464 (this PR)

Description

Implements the new onboarding UI/UX.

Scope

The UI is designed here: https://www.figma.com/file/nscneJs8hvI6j2m24AFTsD/Setup-steps-macOS-Network-Protection?type=design&node-id=132%3A9918&mode=design&t=qrWZTHgdsccYM1IZ-1

My suggestion though would be not to focus on the UI too strongly unless you see a big issue, as this will be run through a design review anyway.

In Scope:

Light mode
Showing the UI that signals the user needs to allow the system extension, before the system extension is allowed
Showing the UI that signals the user needs to allow the VPN configuration, before the VPN configuration is allowed

Out of Scope:

Dark mode
Different images for older macOS versions

Steps to test this PR:

Launch the app
Reset Network Protection's status to make sure you start with a clean state.
Kill the menu app through Activity Monitor at least once
Open the NetP popover and make sure it shows a view that shows how to allow the system extension. The controller part of the view should be disabled.
Click on the button in that view, NetP should start and request that you allow the system extension
Allow it
Now the UI (in the popover and menu app) should be updated to show a view that explains how to allow the VPN configuration. The controller part of the view should be disabled.
Allow it
The onboarding UI should go away and you should see the regular NetP popover status view.

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

… styled)

github-actions · 2023-08-10T09:20:59Z

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

Generated by 🚫 dangerJS against 3eaf24c

…/netp-new-onboarding

diegoreymendez · 2023-08-11T14:31:26Z

DuckDuckGo/Menus/MainMenu.storyboard

-                                                                <menuItem title="Print onboarding status to console" id="abt-dL-pSE">
-                                                                    <modifierMask key="keyEquivalentModifierMask"/>
-                                                                    <connections>
-                                                                        <action selector="printOnboardingStatusToConsoleWithSender:" target="L82-We-Wwv" id="k2z-AC-UBV"/>
-                                                                    </connections>
-                                                                </menuItem>


This was added in the previous PR in the stack for testing purposes.

diegoreymendez · 2023-08-11T14:32:13Z

DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionOnboardingMenu.swift

-    @IBAction
-    func printOnboardingStatusToConsole(sender: NSMenuItem) {
-        print("Onboarding status is: \(String(describing: OnboardingStatus(rawValue: onboardingStatus)))")
-    }


This matches the removal of the debug menu item I added in my last PR. It's no longer needed.

diegoreymendez · 2023-08-11T14:34:52Z

...kProtectionUI/Sources/NetworkProtectionUI/Views/StatusView/NetworkProtectionStatusView.swift

+                if let healthWarning = model.issueDescription {
+                    connectionHealthWarningView(message: healthWarning)
+                }


We now only show these warnings if the onboarding is completed.

diegoreymendez · 2023-08-11T14:35:37Z

...ionUI/Sources/NetworkProtectionUI/Views/TunnelControllerView/TunnelControllerViewModel.swift

+        if !viewDisabled {
+            toggleTransition = .switchingOn(locallyInitiated: true)
+        }


The toggle won't move when onboarding is shown, as the tunnel controller view will be disabled.

We'll still start NetP though.

diegoreymendez · 2023-08-11T14:36:22Z

...tectionUI/Sources/NetworkProtectionUI/Views/OnboardingStepView/OnboardingStepViewModel.swift

+
+    /// Model for AllowSystemExtensionView
+    ///
+    final class Model: ObservableObject {


Model for the onboarding step view.

…go/netp-new-onboarding

diegoreymendez · 2023-08-14T10:30:47Z

I'm going to be merging the parent PR in the stack soon, so there'll be some merging back and forth. That said there will be no changes to the code in this PR, just activity noise. :)

Task/Issue URL: https://app.asana.com/0/0/1205257341292466/f Tech Design URL: CC: ## Stack: - Feature branch: `diego/feature-network-protection-onboarding` - #1474 (this PR) - #1464 ## Description: Implements the shared `UserDefaults` that will store the onboarding status, and the debug menu options to test it.

graeme

Only blocker is the enum as we discussed.

graeme · 2023-08-14T11:18:48Z

LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/NetworkProtectionColor.swift

    case alertBubbleBackground = "AlertBubbleBackground"
    case defaultText = "TextColor"
    case linkColor = "LinkBlueColor"
+    case onboardingButtonBackgroundColor = "OnboardingButtonBackgroundColor"


I wonder if any of these colours could be found in DesignResourcesKit. If we decide to change the app styling globally this could trip us up later. Not a blocker, just potentially a thought for later.

graeme · 2023-08-14T11:23:33Z

LocalPackages/NetworkProtectionUI/Sources/NetworkProtectionUI/SwiftUI/ViewDisabler.swift

+    /// Disables a view giving it opacity and making it impossible to interact with.  Most useful on composite views.
+    ///
+    @ViewBuilder
+    func disabled(on condition: Bool) -> some View {


Doesn’t this clash with the default disabled view modifier? I wonder if this should be named differently. Not a blocker though if you’ve already considered this and it’s okay.

Well I've pondered on this thought but honestly I couldn't find anything better, and unless there's an option to make this better I'd just keep this to be honest.

This does in fact disable the view, and the on: parameter ensures it's clear which one is picked against the regular disabled.

graeme · 2023-08-14T11:31:13Z

.../NetworkProtection/AppAndExtensionAndAgentTargets/UserDefaults+NetworkProtectionShared.swift

+    @objc
+    dynamic var networkProtectionOnboardingStatusRawValue: Int {
+        get {
+            value(forKey: networkProtectionOnboardingStatusRawValueKey) as? Int ?? OnboardingStatus.default.rawValue


Persisting enums as Ints is a bit risky in case someone inadvertently adds a case. Particularly given this one is performing a calculation for the parameter which simply adds 1 to change values. If someone added a case before or after, it will change the meaning of this. I’d recommend using strings for this case.

This is changed as per our private chat.

graeme · 2023-08-14T11:52:44Z

...ectionUI/Sources/NetworkProtectionUI/Views/StatusView/NetworkProtectionStatusViewModel.swift

+
+            onboardingStatusPublisher
+                .receive(on: DispatchQueue.main)
+                .sink { [weak self] status in


You can simplify this by using the .assign(to:onWeaklyHeld:) instead of .sink (not a blocker, just a tip)

Nice call, makes it tidier. Changed.

…ak if there are changes

diegoreymendez · 2023-08-14T12:21:33Z

Feedback addressed.

It seems there's a flaky test which I'll address before merging, but I'd suggest we separate the review approval from CI since I won't merge until the CI is green anyways.

graeme

Tested and works nicely. Thanks for addressing my comments too. Approved! 👍

…/netp-new-onboarding

Task/Issue URL: https://app.asana.com/0/0/1205301030177476/f ## Description: Implements the new onboarding for Network Protection. ## Incremental PR reviews This is a list of the individual PRs (and reviews) that make up this feature: - #1464 - #1466 - #1474 - #1484 - #1502

diegoreymendez added 20 commits August 3, 2023 16:44

WIP

0a06b73

Fixes the multiline text heights

7a14c10

Rolled back one change

92ae90d

Fixes a file's header

88f8b35

Merges the latest from develop

63bb1f1

Made a small change to ensure multiline text height is properly updated

01d8bc3

Updated a comment

062b23b

Merges the latest from diego/fix-multiline-text-height

0996f26

Merges the latest from develop

8b4978f

WIP

6a36b50

WIP

51a625c

WIP

3413a3d

WIP

68ad1e0

WIP

a144943

Implements first version of onboarding view fully working (but poorly…

385e810

… styled)

Several improvements to the onboarding UI

caf1e59

Merges the latest from develop

7354d96

WIP

7351d62

First complete version of the NetP onboarding flow

72a41f3

Separates NetworkProtectionStatusView a bit more

15ab293

diegoreymendez self-assigned this Aug 10, 2023

diegoreymendez changed the base branch from develop to diego/feature-network-protection-onboarding August 10, 2023 09:23

diegoreymendez added 7 commits August 10, 2023 11:23

Merge branch 'diego/feature-network-protection-onboarding' into diego…

527e4f7

…/netp-new-onboarding

Merges the latest from the feature base branch

5b404b3

Merges the latest from the base feature branch

28d5b92

Merges the latest from the base feature branch

622c2a3

Fixes an issue with App Store builds

0af9a02

Implements debug menu options for NetP's onboarding

7fab28b

Adds the status changing calls

e4dc19e

Merges the latest from the base branch

bcc1edf

diegoreymendez changed the base branch from diego/feature-network-protection-onboarding to diego/feature-network-protection-onboarding-3 August 11, 2023 14:30

diegoreymendez commented Aug 11, 2023

View reviewed changes

diegoreymendez changed the title ~~[WIP] Network Protection - Full feature draft PR~~ Implements new onboarding UI/UX Aug 11, 2023

diegoreymendez mentioned this pull request Aug 11, 2023

Implements NetP onboarding UserDefaults and debug menu options #1474

Merged

diegoreymendez requested a review from graeme August 11, 2023 14:45

diegoreymendez marked this pull request as ready for review August 11, 2023 14:46

diegoreymendez added 4 commits August 11, 2023 16:53

Fixes the unit tests

6f0e1d4

Adds a unit test

1ed8f62

Removes some @mainactor requirements

15e471d

Merge branch 'diego/feature-network-protection-onboarding-3' into die…

52b2806

…go/netp-new-onboarding

Base automatically changed from diego/feature-network-protection-onboarding-3 to diego/feature-network-protection-onboarding August 14, 2023 10:36

Merges the latest from the feature branch

ec35765

graeme suggested changes Aug 14, 2023

View reviewed changes

diegoreymendez added 2 commits August 14, 2023 13:54

Makes the onboarding raw value a string to make it less likely to bre…

adb0520

…ak if there are changes

Tidies up some code

2017648

diegoreymendez requested a review from graeme August 14, 2023 12:19

graeme approved these changes Aug 14, 2023

View reviewed changes

Merge branch 'diego/feature-network-protection-onboarding' into diego…

3eaf24c

…/netp-new-onboarding

diegoreymendez merged commit bbd4e29 into diego/feature-network-protection-onboarding Aug 14, 2023

diegoreymendez deleted the diego/netp-new-onboarding branch August 14, 2023 14:01

diegoreymendez mentioned this pull request Aug 18, 2023

Update onboarding-related error states #1504

Merged

Implements new onboarding UI/UX #1464

Implements new onboarding UI/UX #1464

Uh oh!

Conversation

diegoreymendez commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack

Description

Scope

In Scope:

Out of Scope:

Steps to test this PR:

Internal references:

Uh oh!

github-actions bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez commented Aug 14, 2023

Uh oh!

graeme left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegoreymendez commented Aug 14, 2023

Uh oh!

graeme left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

diegoreymendez commented Aug 10, 2023 •

edited

Loading

github-actions bot commented Aug 10, 2023 •

edited

Loading