Design adjustments for Network Protection onboarding views #1484
Conversation
diegoreymendez
changed the title
| onboardingStatusRawValue = OnboardingStatus.isOnboarding(step: .userNeedsToAllowVPNConfiguration).rawValue | ||
| // We'll only update to completed if we were showing the onboarding step to | ||
| // allow the system extension. Otherwise we may override the allow-VPN | ||
| // onboarding step. | ||
| if onboardingStatusRawValue == OnboardingStatus.isOnboarding(step: .userNeedsToAllowExtension).rawValue { | ||
| onboardingStatusRawValue = OnboardingStatus.completed.rawValue | ||
| } |
There was a problem hiding this comment.
This is based on design feedback.
Previous onboarding logic:
- Show "Allow system extension"
- Show "Allow vpn configuration"
- Completed
Current onboarding logic:
- Show "Allow system extension"
- Dismiss onboarding view (ie: mark completed if the system-extension variant of it was shown)
- Show "Allow VPN configuration" only if the user disallows it
- Dismiss all onboarding views on successful completion.
...etworkProtectionUI/Sources/NetworkProtectionUI/Extensions/UserText+NetworkProtectionUI.swift
Show resolved
Hide resolved
|
@ayoy - Could I ask you to check the BigSur assets look fine for this PR? For checking the BigSure assets it's enough to do test steps 1 through 3. |
| .allowSysexScreenshotBigSur: "allow-sysex-screenshot-bigsur" | ||
| ] | ||
|
|
||
| XCTAssertEqual(assetsAndExpectedRawValues.count, NetworkProtectionAsset.allCases.count) |
There was a problem hiding this comment.
This line is a quick way to ensure any new case added to the enum is accounted for here.
|
|
||
| for (asset, rawValue) in assetsAndExpectedRawValues { | ||
| XCTAssertEqual(asset.rawValue, rawValue) | ||
| XCTAssertNotNil(Image(rawValue, bundle: .module)) |
There was a problem hiding this comment.
We're also now making sure that the actual assets are present in the bundle.
There was a problem hiding this comment.
The spaces in this filename are freaking me out 😂 I don't think I have any good reason to be freaked out, just paranoia.
samsymons
left a comment
samsymons
left a comment
There was a problem hiding this comment.
For the happy path this is working very well for me.
As discussed on MM I have an issue where this breaks in the case where you dismiss the prompt without actually clicking "Allow" - I've done a couple clean builds and still see this behaviour.
Tbh, if you're not able to reproduce it then I think it's fine to merge and just keep an eye on this issue, we can worry about it if we see any user reports. It's not like the button not working is a blocker anyway, the UI still shows the user how to proceed, so it's a net improvement.
diegoreymendez
requested review from
graeme
and removed request for
ayoy and
graeme
|
I'm momentarily pulling this back from review, as there's more design feedback. |
…e system extension
…/netp-new-onboarding-3
2 participants
Task/Issue URL: https://app.asana.com/0/0/1205268328062294/f
Description
Addresses design adjustments requested for the new Network Protection onboarding views.
Work done:
Scope:
Address design feedback. Nothing outside of design feedback is in scope for this PR.
The only design change we should validate technically is the BigSur vs Ventura onboarding screenshot changes.
Screenshots
This is the onboarding screen showing the relevant screenshot that changes between macOS versions.
Testing:
This PR will be accompanied by a design-review build, so I'd encourage the technical reviewer to focus on the logical changes mainly (and not so much on coloring, fonts, spacing, etc).
To test this you need to enable NetP by going into Settings > About and tapping 12 times in the version number to insert an invite code.
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation