Add connection detail toggle to connection view #7388

rablador · 2024-12-20T09:00:30Z

The connection details should be shown/hidden depending on if the view is expanded or collapsed, via the tappable chevron. This PR does this, adds expandable state and implements the feature indicators in a feature complete package.

Note: This PR also includes https://linear.app/mullvad/issue/IOS-932/add-feature-indicator-pills-to-connection-details.

To do before release:

☑️ Update E2E tests.
☑️ Update screenshot tests.
☑️ Remove old connection view code, eg. TunnelViewController, TunnelControllView, TunnelState+UI etc.
☑️ Remove DEBUG flow and make connection view available in production (in TunnelCoordinator).

To do at some point:

✅ Touch target for expandable chevron is off and small.
☑️ Position location marker on the map correctly. Maybe align with the loading spinner?
☑️ We should add a status indicator for when loading the outgoing ips.
☑️ We should add animations for expanding/contracting as well as when state changes.
☑️ We should add country and city to connection header when not connected, preferrably with a matching location marker.
🐞 ~~There's a bug when connecting and disconnecting where the buttom part of the connection view loses corner radius and is moved slightly upward. The behavior is inconsistent and require som bug bashing.~~ Can't seem to trigger it anymore...
✅ 🐞 If connecting/disconnecting repeatedly you will mess up the network and disable the connect button. This is by design, but there's a bug where the button does not get reenabled when network recovers.

Other:

ℹ️ Scrollviews cannot be made to not bounce on iOS 15 without hacks, so I left it as it is. This applies to the scrollview wrapping the connection details.

This change is

linear · 2024-12-20T09:00:32Z

IOS-961 Add connection detail toggle to connection view

rablador

Reviewable status: 0 of 32 files reviewed, 2 unresolved discussions

ios/MullvadRustRuntimeTests/MullvadPostQuantum+Stubs.swift line 15 at r1 (raw file):

@testable import WireGuardKitTypes

// swiftlint:disable function_parameter_count

Everything in this file is to make the unit tests work (currently failing when looking for MullvadTypes.WgFuncPointers).

ios/convert-assets.rb line 35 at r1 (raw file):

  "icon-fail.svg",
  "icon-info.svg",
  "icon-reload.svg",

Added as a pure SVG in project assets instead of being converted here.

acb-mv

Reviewed 31 of 32 files at r1, 1 of 4 files at r2.
Reviewable status: 29 of 32 files reviewed, 5 unresolved discussions (waiting on @rablador)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 25 at r1 (raw file):

        for (index, chip) in chips.enumerated() {
            let textWidth = chip.name.width(using: .preferredFont(forTextStyle: .subheadline))
            let chipWidth = textWidth + 16 /* inside horisontal padding */ + 8 /* outside trailing padding */

typo: should be "horizontal"

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipFeature.swift line 24 at r1 (raw file):

    var name: String {
        String("DAITA")

Is the `String("

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift line 85 at r2 (raw file):

    var name: String {
        String("Server IP override")

Is the String("...") wrapper necessary? IIRC,"Server IP override" would return a String

rablador

Reviewable status: 28 of 32 files reviewed, 4 unresolved discussions (waiting on @acb-mv)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipFeature.swift line 24 at r1 (raw file):

Previously, acb-mv wrote…

Is the `String("

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift line 85 at r2 (raw file):

Previously, acb-mv wrote…

Is the String("...") wrapper necessary? IIRC,"Server IP override" would return a String

Done.

mojganii

When we change the tunnel settings , the connection view is expanded which it's not correct.

Reviewable status: 17 of 43 files reviewed, 15 unresolved discussions (waiting on @acb-mv and @rablador)

ios/MullvadVPN/Extensions/View+TapAreaSize.swift line 31 at r7 (raw file):

            )
            .contentShape(Rectangle())
            .frame(width: actualViewSize.width, height: actualViewSize.height)

we are setting frame size twice which isn't necessary, we can remove the second frame property. in addition, since the it's view modifier to prevent form setting size less than minimum then putting .contentShape(Rectangle()) should be done in the component which is using this modifier.
as we discussed offline, if there is a behind reason for that or it has some side effects on the code, let comment it out here:

Code snippet:

private struct TappablePadding: ViewModifier {
    @State private var actualViewSize: CGSize = .zero
    private let tappableViewSize = UIMetrics.Button.minimumTappableAreaSize

    func body(content: Content) -> some View {
        content
            .sizeOfView { actualViewSize = $0 }
            .frame(
                width: max(actualViewSize.width, tappableViewSize.width),
                height: max(actualViewSize.height, tappableViewSize.height)
            )
//            .contentShape(Rectangle())
//            .frame(width: actualViewSize.width, height: actualViewSize.height)
    }
}


struct CustomToggleStyle: ToggleStyle {
    private let width: CGFloat = 48
    private let height: CGFloat = 30
    private let circleRadius: CGFloat = 23

    var disabled = false
    let accessibilityId: AccessibilityIdentifier?
    var infoButtonAction: (() -> Void)?

    func makeBody(configuration: Configuration) -> some View {
        HStack {
            configuration.label
                .opacity(disabled ? 0.2 : 1)

            if let infoButtonAction {
                Button(action: infoButtonAction) {
                    Image(.iconInfo)
                }
                .adjustingTapAreaSize()
                .contentShape(Rectangle())
                .tint(.white)
            }

            Spacer()

            ZStack(alignment: configuration.isOn ? .trailing : .leading) {
                Capsule(style: .circular)
                    .frame(width: width, height: height)
                    .foregroundColor(.clear)
                    .overlay(
                        RoundedRectangle(cornerRadius: circleRadius)
                            .stroke(
                                Color(.white.withAlphaComponent(0.8)),
                                lineWidth: 2
                            )
                    )
                    .opacity(disabled ? 0.2 : 1)

                Circle()
                    .frame(width: circleRadius, height: circleRadius)
                    .padding(3)
                    .foregroundColor(
                        configuration.isOn
                            ? Color(uiColor: UIColor.Switch.onThumbColor)
                            : Color(uiColor: UIColor.Switch.offThumbColor)
                    )
                    .opacity(disabled ? 0.4 : 1)
            }
            .accessibilityIdentifier(accessibilityId?.asString ?? "")
            .onTapGesture {
                toggle(configuration)
            }
            .adjustingTapAreaSize()
        }
    }

    private func toggle(_ configuration: Configuration) {
        withAnimation(.easeInOut(duration: 0.25)) {
            configuration.isOn.toggle()
        }
    }
}

ios/MullvadVPN/Views/MainButtonStyle.swift line 13 at r10 (raw file):

struct MainButtonStyle: ButtonStyle {
    var style: Style
    var disabled: Bool

we can remove disabled property from here and cunstructor.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift line 70 at r10 (raw file):

                    text: LocalizedStringKey(
                        viewModel.tunnelStatus.state == .waitingForConnectivity(.noConnection)
                            ? "Disconnect"

texts should be localized.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 16 at r10 (raw file):

extension ChipViewModelProtocol {
    func chipsToAdd(forContainerWidth containerWidth: CGFloat) -> (chips: [ChipModel], chipsWillOverflow: Bool) {

nit: isOverflowing

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 20 at r10 (raw file):

        var chipsWillOverflow = false

        let moreTextWidth = "\(chips.count) more..."

it should be localized.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 36 at r10 (raw file):

            let chipWillFit = totalChipsWidth <= containerWidth

            if chipWillFitWithMoreText {

can we make it more simplifier :

Code snippet:

            guard (chipWillFit && isLastChip) || chipWillFitWithMoreText else {
                chipsWillOverflow = true
                break
            }
            chipsToAdd.append(chip)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/HeaderView.swift line 48 at r10 (raw file):

                        .foregroundStyle(.white)
                        .accessibilityIdentifier(AccessibilityIdentifier.relayStatusCollapseButton.asString)
                        .transaction { transaction in

Since we don't have any specific animation for the chevron based on desktop app, then we can remove that.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 28 at r10 (raw file):

            // placed in a UIKit context. If ConnectionView would ever be placed as a subview of SwiftUI
            // parent, this reader could probably be removed.
            GeometryReader { _ in

Since the we are calculating the size of content in bottom then I believe we can remove this from here

Code snippet:

 var body: some View {
            Divider()
                .background(UIColor.secondaryTextColor.color)
                .showIf(isExpanded)
            ScrollView {
                VStack(spacing: 16) {
                    FeatureIndicatorsView(
                        viewModel: indicatorsViewModel,
                        isExpanded: $isExpanded
                    )
                    .showIf(!indicatorsViewModel.chips.isEmpty)
                    
                    DetailsView(viewModel: connectionViewModel)
                        .transition(.move(edge: .bottom))
                        .showIf(isExpanded)
                }
                .sizeOfView { scrollViewHeight = $0.height }
            }
            .frame(maxHeight: scrollViewHeight)
        }
    }

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 38 at r10 (raw file):

                        DetailsView(viewModel: connectionViewModel)
                            .transition(.move(edge: .bottom))

there is another issue for animation implementation, it can be removed.
main

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift line 13 at r10 (raw file):

protocol ChipFeature {
    var isEnabled: Bool { get }
    var name: String { get }

I suggest you to add comment here which describes the reason behind using NSLocalizedString instead of LocalizedStringKey.

as we discussed the reason why we are calculating the text size for chips then it matters to have localized value to compute the correct text width.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipView.swift line 14 at r10 (raw file):

    let item: ChipModel
    var body: some View {
        Text(LocalizedStringKey(item.name))

Since we are already using the localized value here, then we need to use the simple format of the text :
Text(item.name)

rablador

Fixed.

Reviewable status: 17 of 43 files reviewed, 13 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPN/Extensions/View+TapAreaSize.swift line 31 at r7 (raw file):

Previously, mojganii wrote…

we are setting frame size twice which isn't necessary, we can remove the second frame property. in addition, since the it's view modifier to prevent form setting size less than minimum then putting .contentShape(Rectangle()) should be done in the component which is using this modifier.
as we discussed offline, if there is a behind reason for that or it has some side effects on the code, let comment it out here:

Done. The .contentShape() needs to stay though, as it's necessary for the increased frame area to be tappable.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipView.swift line 14 at r10 (raw file):

Previously, mojganii wrote…

Since we are already using the localized value here, then we need to use the simple format of the text :
Text(item.name)

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 16 at r10 (raw file):

Previously, mojganii wrote…

nit: isOverflowing

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 20 at r10 (raw file):

Previously, mojganii wrote…

it should be localized.

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 36 at r10 (raw file):

Previously, mojganii wrote…

can we make it more simplifier :

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift line 70 at r10 (raw file):

Previously, mojganii wrote…

texts should be localized.

I have thought about this some more. I think it's fine to keep the LocalizedStringKey that we have in the swiftUI code right now. Perhaps it could be a good way of testing it out. If you feel strongly about it I can "revert" to NSLocalizedString.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 28 at r10 (raw file):

Previously, mojganii wrote…

Since the we are calculating the size of content in bottom then I believe we can remove this from here

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 38 at r10 (raw file):

Previously, mojganii wrote…

there is another issue for animation implementation, it can be removed.
main

Done.

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/HeaderView.swift line 48 at r10 (raw file):

Previously, mojganii wrote…

Since we don't have any specific animation for the chevron based on desktop app, then we can remove that.

Done.

ios/MullvadVPN/Views/MainButtonStyle.swift line 13 at r10 (raw file):

Previously, mojganii wrote…

we can remove disabled property from here and cunstructor.

Done.

Crazy-9669 · 2025-01-09T10:02:34Z

ср, 8 янв. 2025 г., 19:01 mojganii ***@***.***>:

…

***@***.**** requested changes on this pull request. When we change the tunnel settings , the connection view is expanded which it's not correct. *Reviewable <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-:-OG59Apw8rEZdYRXXZXL:b-7r29op>* status: 17 of 43 files reviewed, 15 unresolved discussions (waiting on @acb-mv <https://github.com/acb-mv> and @rablador <https://github.com/rablador>) ------------------------------ *ios/MullvadVPN/Extensions/View+TapAreaSize.swift line 31 at r7 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG4q8IC2AmoCY2xOl7_:-OG4q8IC2AmoCY2xOl7a:bb8flsv> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/c32c22f2066f2ffa153b7b3dd63ed85f98c6a1c9/ios/MullvadVPN/Extensions/View%2BTapAreaSize.swift#L31>):* ) .contentShape(Rectangle()) .frame(width: actualViewSize.width, height: actualViewSize.height) we are setting frame size twice which isn't necessary, we can remove the second frame property. in addition, since the it's view modifier to prevent form setting size less than minimum then putting .contentShape(Rectangle()) should be done in the component which is using this modifier. as we discussed offline, if there is a behind reason for that or it has some side effects on the code, let comment it out here: *Code snippet:* private struct TappablePadding: ViewModifier { @State private var actualViewSize: CGSize = .zero private let tappableViewSize = UIMetrics.Button.minimumTappableAreaSize func body(content: Content) -> some View { content .sizeOfView { actualViewSize = $0 } .frame( width: max(actualViewSize.width, tappableViewSize.width), height: max(actualViewSize.height, tappableViewSize.height) ) // .contentShape(Rectangle()) // .frame(width: actualViewSize.width, height: actualViewSize.height) }} struct CustomToggleStyle: ToggleStyle { private let width: CGFloat = 48 private let height: CGFloat = 30 private let circleRadius: CGFloat = 23 var disabled = false let accessibilityId: AccessibilityIdentifier? var infoButtonAction: (() -> Void)? func makeBody(configuration: Configuration) -> some View { HStack { configuration.label .opacity(disabled ? 0.2 : 1) if let infoButtonAction { Button(action: infoButtonAction) { Image(.iconInfo) } .adjustingTapAreaSize() .contentShape(Rectangle()) .tint(.white) } Spacer() ZStack(alignment: configuration.isOn ? .trailing : .leading) { Capsule(style: .circular) .frame(width: width, height: height) .foregroundColor(.clear) .overlay( RoundedRectangle(cornerRadius: circleRadius) .stroke( Color(.white.withAlphaComponent(0.8)), lineWidth: 2 ) ) .opacity(disabled ? 0.2 : 1) Circle() .frame(width: circleRadius, height: circleRadius) .padding(3) .foregroundColor( configuration.isOn ? Color(uiColor: UIColor.Switch.onThumbColor) : Color(uiColor: UIColor.Switch.offThumbColor) ) .opacity(disabled ? 0.4 : 1) } .accessibilityIdentifier(accessibilityId?.asString ?? "") .onTapGesture { toggle(configuration) } .adjustingTapAreaSize() } } private func toggle(_ configuration: Configuration) { withAnimation(.easeInOut(duration: 0.25)) { configuration.isOn.toggle() } }} ------------------------------ *ios/MullvadVPN/Views/MainButtonStyle.swift line 13 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5dDhU6J9RNQXInjnz:-OG5dDhU6J9RNQXInjo-:bpywh9n> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/Views/MainButtonStyle.swift#L13>):* struct MainButtonStyle: ButtonStyle { var style: Style var disabled: Bool we can remove disabled property from here and cunstructor. ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift line 70 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5EIse6RctkXIVyGzd:-OG5EIse6RctkXIVyGze:b-2a9fby> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift#L70>):* text: LocalizedStringKey( viewModel.tunnelStatus.state == .waitingForConnectivity(.noConnection) ? "Disconnect" texts should be localized. ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 16 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG4ycQH4m6AKNrHUJ2W:-OG4ycQH4m6AKNrHUJ2X:b-tn3y0g> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift#L16>):* extension ChipViewModelProtocol { func chipsToAdd(forContainerWidth containerWidth: CGFloat) -> (chips: [ChipModel], chipsWillOverflow: Bool) { nit: isOverflowing ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 20 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5AhCR9EYlTBIBHXfY:-OG5AhCR9EYlTBIBHXfZ:bx7ikyb> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift#L20>):* var chipsWillOverflow = false let moreTextWidth = "\(chips.count) more..." it should be localized. ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift line 36 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG58a_G27kjtdK0z_8E:-OG58a_G27kjtdK0z_8F:b-6xpc9h> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ChipView/ChipViewModelProtocol.swift#L36>):* let chipWillFit = totalChipsWidth <= containerWidth if chipWillFitWithMoreText { can we make it more simplifier : *Code snippet:* guard (chipWillFit && isLastChip) || chipWillFitWithMoreText else { chipsWillOverflow = true break } chipsToAdd.append(chip) ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/HeaderView.swift line 48 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5PcNRA-d9HS7xge75:-OG5PcNRA-d9HS7xge76:b-njih6h> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ConnectionView/HeaderView.swift#L48>):* .foregroundStyle(.white) .accessibilityIdentifier(AccessibilityIdentifier.relayStatusCollapseButton.asString) .transaction { transaction in Since we don't have any specific animation for the chevron based on desktop app, then we can remove that. ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 28 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5Loiq10z8Sbd1SMkw:-OG5Loiq10z8Sbd1SMkx:b-miae0n> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift#L28>):* // placed in a UIKit context. If ConnectionView would ever be placed as a subview of SwiftUI // parent, this reader could probably be removed. GeometryReader { _ in Since the we are calculating the size of content in bottom then I believe we can remove this from here *Code snippet:* var body: some View { Divider() .background(UIColor.secondaryTextColor.color) .showIf(isExpanded) ScrollView { VStack(spacing: 16) { FeatureIndicatorsView( viewModel: indicatorsViewModel, isExpanded: $isExpanded ) .showIf(!indicatorsViewModel.chips.isEmpty) DetailsView(viewModel: connectionViewModel) .transition(.move(edge: .bottom)) .showIf(isExpanded) } .sizeOfView { scrollViewHeight = $0.height } } .frame(maxHeight: scrollViewHeight) } } ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift line 38 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG5Q-B7-Q-_8tWJjSTp:-OG5Q-B82T-H1G08Gae4:bpi2rv> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ConnectionView/DetailsContainer.swift#L38>):* DetailsView(viewModel: connectionViewModel) .transition(.move(edge: .bottom)) there is another issue for animation implementation, it can be removed. main ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift line 13 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG59YEGBytPOf2yp0ff:-OG59YEH5RtIRqMyB73R:b-5cxmoq> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift#L13>):* protocol ChipFeature { var isEnabled: Bool { get } var name: String { get } I suggest you to add comment here which describes the reason behind using NSLocalizedString instead of LocalizedStringKey. as we discussed the reason why we are calculating the text size for chips then it matters to have localized value to compute the correct text width. ------------------------------ *ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipView.swift line 14 at r10 <https://reviewable.io/reviews/mullvad/mullvadvpn-app/7388#-OG53PVYAfnLiDOCVUIb:-OG53PVYAfnLiDOCVUIc:b8l0h9w> (raw file <https://github.com/mullvad/mullvadvpn-app/blob/497bcd4a4f566885c87b401b4849b0cd610d2cf9/ios/MullvadVPN/View%20controllers/Tunnel/FeatureIndicators/ChipView/ChipView.swift#L14>):* let item: ChipModel var body: some View { Text(LocalizedStringKey(item.name)) Since we are already using the localized value here, then we need to use the simple format of the text : Text(item.name) — Reply to this email directly, view it on GitHub <#7388 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BE55V3UA2TA6PZYF5UTLVJ32JVDVZAVCNFSM6AAAAABT6WKRRWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMZXGU2TSNBVHA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

rablador

Reviewable status: 16 of 43 files reviewed, 13 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipView/ChipFeature.swift line 13 at r10 (raw file):

Previously, mojganii wrote…

I suggest you to add comment here which describes the reason behind using NSLocalizedString instead of LocalizedStringKey.

as we discussed the reason why we are calculating the text size for chips then it matters to have localized value to compute the correct text width.

Done.

mojganii

Reviewable status: 16 of 43 files reviewed, 7 unresolved discussions (waiting on @acb-mv and @rablador)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift line 70 at r10 (raw file):

Previously, rablador (Jon Petersson) wrote…

I have thought about this some more. I think it's fine to keep the LocalizedStringKey that we have in the swiftUI code right now. Perhaps it could be a good way of testing it out. If you feel strongly about it I can "revert" to NSLocalizedString.

No that's fine, after carful consideration I came to the conclusion it's better to take advantage of new APIs as much as we can.

rablador

Reviewable status: 16 of 43 files reviewed, 7 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView/ButtonPanel.swift line 70 at r10 (raw file):

Previously, mojganii wrote…

No that's fine, after carful consideration I came to the conclusion it's better to take advantage of new APIs as much as we can.

Done.

mojganii

Reviewable status: 15 of 43 files reviewed, 7 unresolved discussions (waiting on @acb-mv)

buggmagnet

It seems that the "X more ...." function does not work on iOS 16.7.0

Reviewed 12 of 32 files at r1, 1 of 4 files at r2, 3 of 10 files at r6, 1 of 11 files at r7, 1 of 9 files at r8, 2 of 11 files at r11.
Reviewable status: 21 of 43 files reviewed, 8 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FI_TunnelViewController.swift line 94 at r11 (raw file):

        }

        interactor.didGetOutGoingAddress = { [weak self] connectionInfo in

nit
outgoing is a single word, it should be didGetOutgoingAddress

rablador added the iOS Issues related to iOS label Dec 20, 2024

rablador requested a review from acb-mv December 20, 2024 09:00

rablador self-assigned this Dec 20, 2024

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from ed4c80d to 5d3630b Compare December 20, 2024 09:02

rablador marked this pull request as draft December 20, 2024 09:11

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch 3 times, most recently from f9a3da3 to 3e1c133 Compare December 20, 2024 10:30

rablador commented Dec 20, 2024

View reviewed changes

acb-mv reviewed Dec 20, 2024

View reviewed changes

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 3e1c133 to e14ae14 Compare December 20, 2024 10:55

rablador commented Dec 20, 2024

View reviewed changes

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch 3 times, most recently from 084940c to 750b18f Compare December 20, 2024 12:46

rablador requested a review from acb-mv December 20, 2024 12:46

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 750b18f to fd11f54 Compare December 20, 2024 14:47

rablador marked this pull request as ready for review December 20, 2024 14:51

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch 2 times, most recently from 0633867 to 118080b Compare December 20, 2024 14:54

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch 4 times, most recently from 64cc3ee to 497bcd4 Compare January 7, 2025 12:54

mojganii requested changes Jan 8, 2025

View reviewed changes

rablador commented Jan 9, 2025

View reviewed changes

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 497bcd4 to 20ddffe Compare January 9, 2025 09:57

mojganii and others added 2 commits January 9, 2025 10:58

Add FeatureIndicatorsView

af50086

Add toggle in connection view

ee2a634

Split ConnectionView, improve previews, attempt animation

7692f33

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 20ddffe to 8f0b812 Compare January 9, 2025 09:58

rablador commented Jan 9, 2025

View reviewed changes

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 8f0b812 to 2ce06b4 Compare January 9, 2025 10:24

mojganii previously approved these changes Jan 9, 2025

View reviewed changes

rablador commented Jan 9, 2025

View reviewed changes

rablador dismissed mojganii’s stale review via 0e6ab0e January 9, 2025 12:36

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch 4 times, most recently from 67648dd to 9925f53 Compare January 9, 2025 13:03

Fix multiple small issues in ConnectionView

702540f

rablador force-pushed the add-connection-detail-toggle-to-connection-view-ios-961 branch from 9925f53 to 702540f Compare January 9, 2025 13:06

mojganii approved these changes Jan 9, 2025

View reviewed changes

buggmagnet reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection detail toggle to connection view #7388

Add connection detail toggle to connection view #7388

rablador commented Dec 20, 2024 •

edited

Loading

linear bot commented Dec 20, 2024

rablador left a comment

acb-mv left a comment

rablador left a comment

mojganii left a comment

rablador left a comment

Crazy-9669 commented Jan 9, 2025 via email

rablador left a comment

mojganii left a comment

rablador left a comment

mojganii left a comment

buggmagnet left a comment

Add connection detail toggle to connection view #7388

Are you sure you want to change the base?

Add connection detail toggle to connection view #7388

Conversation

rablador commented Dec 20, 2024 • edited Loading

linear bot commented Dec 20, 2024

rablador left a comment

Choose a reason for hiding this comment

acb-mv left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

Crazy-9669 commented Jan 9, 2025 via email

rablador left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

rablador commented Dec 20, 2024 •

edited

Loading