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

VPN Geoswitching - initial draft #1978

Merged
merged 6 commits into from
Dec 15, 2023
Merged

VPN Geoswitching - initial draft #1978

merged 6 commits into from
Dec 15, 2023

Conversation

graeme
Copy link
Collaborator

@graeme graeme commented Dec 15, 2023

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

Description:

This adds an initial implementation of location switching for the VPN. Known issues:

  • Some of the layout / spacing is not quite right
  • The tap area of the items on the VPN Location selection screen is too small
  • The styling of the list sections on the VPN Location screen is not finished.
  • There seems to be a bug where the initial selection is not respected when connecting after selecting
  • There is a momentary delay before the list items load.
  • Needs unit tests (I promise I will add them, they are very similar to the iOS ones so it won’t take long. I just ran out of time before the holidays).

Steps to test this PR:

  1. Make sure you’re an internal user
  2. Navigate to Settings -> VPN
  3. Check this screen and the VPN Location that the Location item links to match the designs
  4. Check that switching countries and/or cities updates the VPN location

Internal references:

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

@graeme graeme requested a review from samsymons December 15, 2023 17:23
@graeme graeme marked this pull request as ready for review December 15, 2023 17:23
Copy link
Contributor

github-actions bot commented Dec 15, 2023

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

Generated by 🚫 dangerJS against a78b802

* main: (48 commits)
  Add additional VPN startup pixels (#1975)
  Updates to Autofill Logins copy (#1924)
  Bump version to 1.69.0 (95)
  Remove the reconnect/disconnect logic from the connection tester (#1970)
  Set marketing version to 1.69.0
  Update embedded files
  DBP: Send internal user param for dbp waitlist pixels (#1972)
  Move release task to proper section in Code Freeze workflow (#1977)
  drop Main.storyboard (#1944)
  Add GHA workflow to cut release branch (#1976)
  Move DBP tests into main target (#1974)
  Use static date for PixelKit tests (#1973)
  Remove DBP test target (#1961)
  Fix date generator for time machine (#1969)
  Improve sync set up error handling (#1966)
  remove QR code from save recovery PDF view (#1968)
  change order of items in autofill add new item (#1967)
  Fix PR Checks workflow (#1962)
  Bump version to 1.68.0 (93)
  Update embedded files
  ...
Comment on lines +230 to +243
extension View {
/// Applies the given transform if the given condition evaluates to `true`.
/// - Parameters:
/// - condition: The condition to evaluate.
/// - transform: The transform to apply to the source `View`.
/// - Returns: Either the original `View` or the modified `View` if the condition is `true`.
@ViewBuilder func `if`<Content: View>(_ condition: Bool, transform: (Self) -> Content) -> some View {
if condition {
transform(self)
} else {
self
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could move this into a View extension so it can be reused by others. I'm not gonna block on it here, I'd rather get this PR in and tidy things up with a follow-up PR once you're back.

Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

This looks good, barring the known issues. Since this is internal only we'll merge it now and come back to this later to finish up. Thanks!

@samsymons samsymons merged commit 2982a23 into main Dec 15, 2023
14 checks passed
@samsymons samsymons deleted the graeme/geoswitching branch December 15, 2023 21:24
samsymons added a commit that referenced this pull request Dec 19, 2023
* main:
  DBP: Fix unreliable date tests (#1981)
  Add search retention pixel for NetP (#1964)
  Sabrina/sync e2e tests (#1959)
  swiftlint build plugin (#1318)
  VPN Geoswitching - initial draft (#1978)
  Add additional VPN startup pixels (#1975)
  Updates to Autofill Logins copy (#1924)
  Bump version to 1.69.0 (95)
  Remove the reconnect/disconnect logic from the connection tester (#1970)
  Set marketing version to 1.69.0
  Update embedded files
  DBP: Send internal user param for dbp waitlist pixels (#1972)
  Move release task to proper section in Code Freeze workflow (#1977)
  drop Main.storyboard (#1944)
  Add GHA workflow to cut release branch (#1976)
  Move DBP tests into main target (#1974)
  Use static date for PixelKit tests (#1973)
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