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

Add feature indicator pills to connection details #7303

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Dec 9, 2024

This pull request introduces a Feature Indicator View that visually displays the enabled settings for the current connection.


This change is Reviewable

Copy link

linear bot commented Dec 9, 2024

@mojganii mojganii added the iOS Issues related to iOS label Dec 9, 2024
@mojganii mojganii changed the base branch from main to add-foundation-for-connection-view-ios-959 December 9, 2024 12:48
@mojganii mojganii force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from a84a1da to f6743b9 Compare December 10, 2024 10:43
@mojganii mojganii changed the title Add feature indicator pills to connection details ios 932 Add feature indicator pills to connection details Dec 10, 2024
@rablador rablador force-pushed the add-foundation-for-connection-view-ios-959 branch 5 times, most recently from 0089905 to 0526bd6 Compare December 11, 2024 13:40
Base automatically changed from add-foundation-for-connection-view-ios-959 to main December 11, 2024 13:43
@mojganii mojganii force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from f6743b9 to 559164a Compare December 11, 2024 14:13
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 10 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)


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

                    lineWidth: 1
                )
                .background(

I think we can set fill (and radius if needed) directly on the rounded rectangle instead.


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FeaturesIndicatoresView.swift line 25 at r2 (raw file):

                }

                ScrollView {

This view should not be scrollable. Instead the connection details view should be scrollable.


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

protocol ChipFeature {
    var isEnabled: Bool { get }
    func name() -> LocalizedStringKey

Nit: Can be a var instead.


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ConnectionView.swift line 20 at r2 (raw file):

            VStack(alignment: .leading, spacing: 16) {
                ConnectionPanel()
                FeaturesIndicatoresView(viewModel: FeaturesIndicatoresMockViewModel())

Nit: Typo - FeatureIndicatorsView

@buggmagnet buggmagnet force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from 559164a to 4dc7f05 Compare December 12, 2024 10:00
@buggmagnet buggmagnet requested a review from rablador December 12, 2024 10:31
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 10 files at r1, 3 of 7 files at r2, 2 of 3 files at r3.
Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FeatureChipViewModel.swift line 11 at r3 (raw file):

import Foundation
import MullvadSettings
import SwiftUI

No need for that import


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipsView/ChipModel.swift line 13 at r3 (raw file):

struct ChipModel: Identifiable {
    let id = UUID()

I wonder if we could be using the name of the chip as a unique identifier instead ?

Copy link
Contributor

@rablador rablador left a 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 r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii)

@rablador rablador requested a review from buggmagnet December 13, 2024 08:21
Copy link
Contributor

@rablador rablador left a 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, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/FeatureChipViewModel.swift line 11 at r3 (raw file):

Previously, buggmagnet wrote…

No need for that import

Done.


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipsView/ChipModel.swift line 13 at r3 (raw file):

Previously, buggmagnet wrote…

I wonder if we could be using the name of the chip as a unique identifier instead ?

Since name is a localized key it would be a little convoluted to turn it back into a string and then use as ID.

@buggmagnet buggmagnet requested a review from rablador December 13, 2024 08:57
buggmagnet
buggmagnet previously approved these changes Dec 13, 2024
Copy link
Contributor

@buggmagnet buggmagnet left a 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, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/View controllers/Tunnel/FeatureIndicators/ChipsView/ChipModel.swift line 13 at r3 (raw file):

Previously, rablador (Jon Petersson) wrote…

Since name is a localized key it would be a little convoluted to turn it back into a string and then use as ID.

Actually nevermind, Identifiable uses ObjectIdentifier to keep track of things which would require having a class for this to work.

@rablador rablador requested a review from buggmagnet December 13, 2024 09:07
@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from 4dc7f05 to 4e77627 Compare December 13, 2024 09:15
rablador
rablador previously approved these changes Dec 13, 2024
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)

@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from 4e77627 to 98323d8 Compare December 13, 2024 09:24
rablador
rablador previously approved these changes Dec 13, 2024
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)

@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from 98323d8 to 8915d93 Compare December 13, 2024 13:42
Copy link
Contributor

@rablador rablador left a 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 r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet and @mojganii)

@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch 6 times, most recently from 7bf59d5 to 2312930 Compare December 16, 2024 22:33
@rablador rablador requested a review from acb-mv December 16, 2024 22:34
@rablador rablador assigned rablador and unassigned mojganii Dec 16, 2024
@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch from 2312930 to 55ede8b Compare December 16, 2024 22:38
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 20 of 20 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)


ios/MullvadVPN/Views/MainButtonStyle.swift line 1 at r7 (raw file):

//

Opportunistically fixed some colors while I was at it.

@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch 2 times, most recently from f6e8d4f to 68d48c9 Compare December 16, 2024 23:30
@rablador rablador force-pushed the add-feature-indicator-pills-to-connection-details-ios-932 branch 3 times, most recently from 13adf0a to 49f83d2 Compare December 16, 2024 23:37
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 21 of 21 files at r10, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

@rablador rablador closed this Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants