-
Notifications
You must be signed in to change notification settings - Fork 368
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
IOS-995 Show exit constraints in connect view #7495
Conversation
66af87a
to
d351c02
Compare
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.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @acb-mv)
ios/MullvadVPNTests/MullvadVPN/View controllers/Tunnel/DestinationDescriberTests.swift
line 89 at r1 (raw file):
} func testDescribeCountryDest() {
Nit: Full function names.
ios/MullvadREST/Relay/RelayWithLocation.swift
line 61 at r1 (raw file):
locations: [String: REST.ServerLocation] ) -> [RelayWithLocation<T>] { /// thiis used to be `RelaySelector.mapRelays`
I think it's ok to omit this comment.
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 15 at r1 (raw file):
protocol DestinationDescribing { func describe(_ loc: UserSelectedRelays) -> String?
Nit: I'd prefer using the full location
name here and below.
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.
Reviewed 9 of 14 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @acb-mv and @rablador)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 15 at r1 (raw file):
Not even a nit, the swift guidelines are pretty explicit about this
Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.
ios/MullvadVPNTests/MullvadVPN/View controllers/Tunnel/DestinationDescriberTests.swift
line 20 at r1 (raw file):
cachedRelays: CachedRelays( relays: .init( locations: [
No need to create another list of fake relays, we can use ServerRelaysResponseStubs.sampleRelays
here instead
ios/MullvadVPNTests/MullvadVPN/View controllers/Tunnel/DestinationDescriberTests.swift
line 85 at r1 (raw file):
XCTAssertEqual( describer.describe(.init(locations: [], customListSelection: .init(listId: listid, isList: true))), "NameOfList"
Please check the desktop behaviour with custom lists, name list is only displayed when the list itself is selected, if you select a sub location in a list, it will be the location name being displayed instead.
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 42 at r1 (raw file):
switch locationSpec { case .country: serverLocation.country case .city: "\(serverLocation.city), \(serverLocation.country)"
We should double check with the desktop team, currently they only show the city name without the country next to it.
See attached screenshot
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.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @acb-mv and @rablador)
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 42 at r1 (raw file):
Previously, buggmagnet wrote…
We should double check with the desktop team, currently they only show the city name without the country next to it.
See attached screenshot
@waahlnaden test ping ?
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.
Reviewable status: 11 of 14 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @rablador, and @waahlnaden)
ios/MullvadREST/Relay/RelayWithLocation.swift
line 61 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
I think it's ok to omit this comment.
Done.
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 15 at r1 (raw file):
Previously, buggmagnet wrote…
Not even a nit, the swift guidelines are pretty explicit about this
Avoid abbreviations. Abbreviations, especially non-standard ones, are effectively terms-of-art, because understanding depends on correctly translating them into their non-abbreviated forms.
It's not actually part of the function signature, but fair enough
ios/MullvadVPN/View controllers/Tunnel/ConnectionView/DestinationDescriber.swift
line 42 at r1 (raw file):
Previously, buggmagnet wrote…
@waahlnaden test ping ?
Fixed
ios/MullvadVPNTests/MullvadVPN/View controllers/Tunnel/DestinationDescriberTests.swift
line 20 at r1 (raw file):
Previously, buggmagnet wrote…
No need to create another list of fake relays, we can use
ServerRelaysResponseStubs.sampleRelays
here instead
Done
ios/MullvadVPNTests/MullvadVPN/View controllers/Tunnel/DestinationDescriberTests.swift
line 85 at r1 (raw file):
Previously, buggmagnet wrote…
Please check the desktop behaviour with custom lists, name list is only displayed when the list itself is selected, if you select a sub location in a list, it will be the location name being displayed instead.
OK, this has been fixed
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.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @waahlnaden)
6e881c6
to
fb42c1d
Compare
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.
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @waahlnaden)
fb42c1d
to
9978e91
Compare
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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @waahlnaden)
a8a66cc
to
f27c416
Compare
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.
Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @waahlnaden)
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.
Reviewed 2 of 14 files at r1, 1 of 3 files at r2, 1 of 6 files at r3, 1 of 1 files at r4, 6 of 6 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
f27c416
to
2a17914
Compare
This updates the Connect button to show the name of the selected destination. If the user has selected a custom list, this will be the list name, otherwise depending on the relays chosen, it will be in the format "Country", "City, Country" or "City (hostname)".
This change is