-
Notifications
You must be signed in to change notification settings - Fork 416
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
Show VPN onboarding tips #3429
Conversation
let statusViewModel: NetworkProtectionStatusViewModel | ||
|
||
init() { | ||
@StateObject |
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.
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 |
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 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 { |
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.
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)) |
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.
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)) | ||
} |
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.
Convenience method to style tip views in a way they can be easily embedded in a list while following the style of other cells.
DuckDuckGo/TipKit/TipGroup.swift
Outdated
protocol TipGrouping { | ||
@available(iOS 17.0, *) | ||
@MainActor | ||
var currentTip: (any Tip)? { get } | ||
} |
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.
This protocol allows us to use TipGrouping
properties that build across all iOS versions.
DuckDuckGo/TipKit/TipGroup.swift
Outdated
struct EmptyTipGroup: TipGrouping { | ||
@available(iOS 17.0, *) | ||
var currentTip: (any Tip)? { | ||
return nil | ||
} | ||
} |
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.
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.
DuckDuckGo/TipKit/TipGroup.swift
Outdated
/// | ||
@available(iOS 17.0, *) | ||
@available(iOS, obsoleted: 18.0) | ||
struct LegacyTipGroup: TipGrouping { |
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.
This is the backport of TipGroup
for iOS 17.
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() | ||
} | ||
}() | ||
|
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.
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.
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.
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
@Bunn - Ready for another look. Most changes are aesthetic at this point. |
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.
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)) |
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.
Since the size repeats here and in the snoozetip, maybe a const would be good.
# 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
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
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