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

Show VPN onboarding tips #3429

Merged
merged 38 commits into from
Oct 28, 2024
Merged

Show VPN onboarding tips #3429

merged 38 commits into from
Oct 28, 2024

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Oct 11, 2024

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

macOS PR: duckduckgo/macos-browser#3410
BSK PR: duckduckgo/BrowserServicesKit#1024

Description

Shows VPN onboarding tips.

Testing

Debug helper option

If you want to reset TipKit, go to More Menu > All debug options > Reset TipKit
Once that's done you'll need to relaunch the app

Steps

  1. Run the app
  2. Enable PrivacyPro if you haven't
  3. Ensure the VPN is OFF before starting
  4. Go to More Menu > VPN
  5. No tips should be visible
  6. Turn the VPN ON.
  7. You should see the "Change your location" tip with no actions. It should be possible to dismiss the tip by either clicking on "X" in the top-right corner or by changing your location through the cell right above.
  8. Once the previous tip is disabled, if the VPN is still ON, you should see a snooze tip with a "Learn more" option. This can be dismissed directly, or by snoozing the connection.
  9. Once that's dismissed you should not see other tips until you turn the connection OFF.
  10. Once the connection is OFF you should see a tip to add a widget. This can be dismissed by tapping "Add widget" which should take you to a separate education screen.

Testing Variant: Older iOS versions support

You can repeat the test below changing the TipGroup implementation used. This can be easily done by changing the OS checks here.

Make sure that when using the iOS 17 implementation all works the same.
Make sure that when using the implementation for older iOS versions you don't see any tips at all anywhere.

Definition of Done (Internal Only):


Internal references:

Software Engineering Expectations
Technical Design Template

let statusViewModel: NetworkProtectionStatusViewModel

init() {
@StateObject
Copy link
Contributor Author

@diegoreymendez diegoreymendez Oct 11, 2024

Choose a reason for hiding this comment

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

Should've been a @StateObject all along since it was owned by this view.

Looking even beyond, this should probably be instantiated elsewhere but that's far beyond the scope of this PR.


struct NetworkProtectionStatusView: View {
@Environment(\.colorScheme) var colorScheme

@StateObject public var statusModel: NetworkProtectionStatusViewModel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The model wasn't owned by this instance, so changed to @ObservedObject.

@@ -151,8 +176,8 @@ struct NetworkProtectionStatusView: View {

@ViewBuilder
private func locationDetails() -> some View {
if !statusModel.isSnoozing, let location = statusModel.location {
Section {
Section {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code a bit to move the section out of the if conditional block, as it was shared in both scenarios.

This cleans up the code a bit and allows me to add the same TipView below for both branches of the if condition.

}
.listRowBackground(Color(designSystemColor: .surface))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the red code has just been moved "out" of the if condition.

///
func removeGroupedListStyleInsets() -> some View {
listRowInsets(EdgeInsets(top: 0, leading: 0, bottom: 0, trailing: 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.

Convenience method to style tip views in a way they can be easily embedded in a list while following the style of other cells.

Comment on lines 23 to 27
protocol TipGrouping {
@available(iOS 17.0, *)
@MainActor
var currentTip: (any Tip)? { get }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This protocol allows us to use TipGrouping properties that build across all iOS versions.

Comment on lines 34 to 39
struct EmptyTipGroup: TipGrouping {
@available(iOS 17.0, *)
var currentTip: (any Tip)? {
return nil
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the struct to instantiate when the OS is lower than iOS 17. It's a glorified no-op.

currentTip is guarded to iOS 17 because we really don't want it used at all, but is necessary in the protocol.

///
@available(iOS 17.0, *)
@available(iOS, obsoleted: 18.0)
struct LegacyTipGroup: TipGrouping {
Copy link
Contributor Author

@diegoreymendez diegoreymendez Oct 11, 2024

Choose a reason for hiding this comment

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

This is the backport of TipGroup for iOS 17.

@diegoreymendez diegoreymendez self-assigned this Oct 11, 2024
@diegoreymendez diegoreymendez requested a review from Bunn October 11, 2024 10:46
@diegoreymendez diegoreymendez marked this pull request as ready for review October 11, 2024 10:47
Comment on lines 114 to 134
let vpnEnabledTips: TipGrouping = {
// This is temporarily disabled until Xcode 16 is available.
// Ref: https://app.asana.com/0/414235014887631/1208528787265444/f
//
// if #available(iOS 18.0, *) {
// return TipGroup(.ordered) {
// VPNChangeLocationTip()
// VPNUseSnoozeTip()
// VPNAddWidgetTip()
// }
if #available(iOS 17.0, *) {
return LegacyTipGroup(.ordered) {
VPNChangeLocationTip()
VPNUseSnoozeTip()
VPNAddWidgetTip()
}
} else {
return EmptyTipGroup()
}
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is great because we'll be using the default thing in iOS 18+, my backport in iOS 17 and a glorified no-op in lower OS versions.

The iOS 18 code is commented for now as Xcode 15.4 fails to build with it. I've created a follow-up task to re-enable.

Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

LGTM.
The lack of animations when displaying the tips is a bit distracting, as every other cell animates due to state changes. I'm not sure if it's related to the tips themselves or something else. Have you tried changing the state that presents the tips with a withAnimation block?. However, as far as functionality goes, it looks good to me.

Video.MP4

@diegoreymendez
Copy link
Contributor Author

@Bunn - Ready for another look. Most changes are aesthetic at this point.

@diegoreymendez diegoreymendez requested a review from Bunn October 22, 2024 15:52
Copy link
Contributor

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

LGTM, but would be good to find someone with an iOS 17 device.

Working as expected for iOS 18
https://github.com/user-attachments/assets/ac215983-a133-447d-9272-0c30dc9095c9

Section {
if #available(iOS 17.0, *) {
widgetTipView()
.tipImageSize(CGSize(width: 32, height: 32))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the size repeats here and in the snoozetip, maybe a const would be good.

@diegoreymendez diegoreymendez merged commit dc67e00 into main Oct 28, 2024
13 checks passed
@diegoreymendez diegoreymendez deleted the diego/add-vpn-tips branch October 28, 2024 14:02
samsymons added a commit that referenced this pull request Oct 29, 2024
# By Mariusz Śpiewak (3) and others
# Via Elle Sullivan (2) and others
* main: (26 commits)
  Release 7.143.0-0 (#3497)
  Fix the condition (#3496)
  Use cookie to share subscription access token on DDG domains (#3488)
  Show VPN onboarding tips (#3429)
  TestFlight only release and promotion (#3474)
  point to BSK branch (#3492)
  Remove Recovered References (#3457)
  Fix image set warning (#3459)
  Make running the script more explicit (#3484)
  Add pixel when the error page is shown (#3487)
  Release 7.142.0-1 (#3483)
  Merge Hotfix/7.141.1 into 7.142.0 release branch (#3481)
  Release 7.141.1-0 (#3479)
  Handle 'data' scheme downloads through WebKit's `decisionHandler(.download)` (#3478)
  Email parsing improved (#3436)
  Fix updating OmniBar constraints on iOS18 (#3473)
  For BLOB downloads make sure the file is not preview-able before triggering the download (#3472)
  Set UserDefaults API access reason to 1C8F.1 (#3475)
  Bump BSK to the version with newTabSearchField feature flag (#3449)
  Use New Tab Page implementation without customization features (#3453)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
diegoreymendez added a commit to duckduckgo/macos-browser that referenced this pull request Nov 28, 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.

3 participants