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 preliminary settings page for relay IP overrides #5688

Merged

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Jan 15, 2024

Currently, there is no bespoke iOS design for this, but that will not stop us. We should follow the desktop UI mockups for now.


This change is Reviewable

Copy link

linear bot commented Jan 15, 2024

@rablador rablador force-pushed the add-preliminary-settings-page-for-relay-ip-overrides-ios-450 branch from cf3e457 to 9157cb8 Compare January 16, 2024 09:09
@rablador rablador added the iOS Issues related to iOS label Jan 16, 2024
Copy link
Collaborator

@mojganii mojganii 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: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @rablador)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 13 at r1 (raw file):

import UIKit

class IPOverrideCoordinator: Coordinator, Presenting, SettingsChildCoordinator {

we can eliminate conforming Presenting for now but if you think we need to present modal top of that in the future, let's keep it that


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 15 at r1 (raw file):

        let view = UIStackView()
        view.axis = .vertical
        view.spacing = 20

try to use the values in UIMetrics or make the custom layout for this


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 62 at r1 (raw file):

        infoButton.tintColor = .white
        infoButton.setImage(UIImage(resource: .iconInfo), for: .normal)
        infoButton.heightAnchor.constraint(equalToConstant: 24).isActive = true

use the padding in UIMetrics


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 66 at r1 (raw file):

        let headerView = UIStackView(arrangedSubviews: [label, infoButton, UIView()])
        headerView.spacing = 8

the same goes for here

@rablador rablador force-pushed the add-preliminary-settings-page-for-relay-ip-overrides-ios-450 branch from 9157cb8 to e6c0ca3 Compare January 16, 2024 15:32
Copy link
Contributor Author

@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: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @mojganii)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideCoordinator.swift line 13 at r1 (raw file):

Previously, mojganii wrote…

we can eliminate conforming Presenting for now but if you think we need to present modal top of that in the future, let's keep it that

We will need it in the next steps when we present document picker and text input view controller.


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 15 at r1 (raw file):

Previously, mojganii wrote…

try to use the values in UIMetrics or make the custom layout for this

This is a very specific margin for this view only. Do we need to "pollute" the UIMetrics with non-generic margins? This answer goes for all the similar comments below.

@rablador rablador force-pushed the add-preliminary-settings-page-for-relay-ip-overrides-ios-450 branch from e6c0ca3 to 61e37dd Compare January 19, 2024 16:18
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Good job!

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 15 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This is a very specific margin for this view only. Do we need to "pollute" the UIMetrics with non-generic margins? This answer goes for all the similar comments below.

do we agree to keep specific spacings and layout constants in that view? if we are going to put shared values in UIMetrics then it's true. but I'm not sure if we agreed or not

Copy link
Contributor Author

@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: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @mojganii)


ios/MullvadVPN/Coordinators/Settings/IPOverride/IPOverrideViewController.swift line 15 at r1 (raw file):

Previously, mojganii wrote…

do we agree to keep specific spacings and layout constants in that view? if we are going to put shared values in UIMetrics then it's true. but I'm not sure if we agreed or not

I'm not sure to be honest. I think I'd rather have shared values in UIMetrics, but we haven't really discussed it properly. I'll start a topic in the ios channel so that we can have a chat about it.

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.

:lgtm:

Reviewed 4 of 8 files at r1, 2 of 3 files at r2, 3 of 4 files at r4, all commit messages.
Reviewable status: 8 of 9 files reviewed, 3 unresolved discussions (waiting on @mojganii)

@buggmagnet buggmagnet force-pushed the add-preliminary-settings-page-for-relay-ip-overrides-ios-450 branch from 61e37dd to 7ad86ce Compare January 23, 2024 15:35
@buggmagnet buggmagnet merged commit e0307dd into main Jan 23, 2024
4 of 5 checks passed
@buggmagnet buggmagnet deleted the add-preliminary-settings-page-for-relay-ip-overrides-ios-450 branch January 23, 2024 15:37
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.

3 participants