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

Make the Voucher view more comforming to designs #5165

Merged

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Sep 19, 2023

This PR makes the voucher view closer to the originally intended design.
A title has been added, a switch for a compactMode where the view is now shown fully (when presented from the account view)
Fonts and font weights have been changed, as well as some margins.


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Sep 19, 2023
@buggmagnet buggmagnet requested a review from rablador September 19, 2023 15:13
@buggmagnet buggmagnet self-assigned this Sep 19, 2023
@linear
Copy link

linear bot commented Sep 19, 2023

IOS-311 Nonconformant design voucher submission dialog in new account flow

The bottoms on the bottom of the voucher redemption screen in the new account flow are misaligned - the bottom margin seems to be too large after dismissing the keyboard.

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.

The margins in compact mode are not correct, they should be aligned with alert margins. See https://mullvad.slack.com/archives/C052D9X882U/p1694419482421749 for discussion and https://reviewable.io/reviews/mullvad/mullvadvpn-app/5152 for a possible solution.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/UI appearance/UIMetrics.swift line 133 at r1 (raw file):

    static let padding4: CGFloat = 4
    static let padding8: CGFloat = 8
    static let padding10: CGFloat = 10

This isn't a criticism of the newly added padding10 but rather of the whole idea of having a corresponding number for each conceivable padding. This will not contribute to a layout with standardised margins and padding, but rather a more cumbersome free-for-all.


ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 44 at r1 (raw file):

            comment: ""
        )
        label.textColor = .white

No need to disable translatesAutoresizingMaskIntoConstraints explicitly. This happens automatically when view is added to a stack view. Same goes for all the other views below.


ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 52 at r1 (raw file):

    private let enterVoucherLabel: UILabel = {
        let label = UILabel()
        label.font = .systemFont(ofSize: 15, weight: .semibold)

Why not use preferredFont here? (and everywhere) We even have an extension with support for weight.

Copy link
Contributor Author

@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, 3 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/UI appearance/UIMetrics.swift line 133 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

This isn't a criticism of the newly added padding10 but rather of the whole idea of having a corresponding number for each conceivable padding. This will not contribute to a layout with standardised margins and padding, but rather a more cumbersome free-for-all.

I agree, but that's what the designs specify, and I didn't want to introduce a magic number


ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 44 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

No need to disable translatesAutoresizingMaskIntoConstraints explicitly. This happens automatically when view is added to a stack view. Same goes for all the other views below.

That's good to know, I'll modify those


ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 52 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why not use preferredFont here? (and everywhere) We even have an extension with support for weight.

Because I couldn't get it to look like the designs. However, I can do this
label.font = .preferredFont(forTextStyle: .body, weight: .semibold).withSize(15) Which is visually the same as systemFont so I'll go with that

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: :shipit: complete! all files reviewed, all discussions resolved

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 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/MullvadVPN/View controllers/RedeemVoucher/RedeemVoucherContentView.swift line 280 at r2 (raw file):

    private func configureUI() {
        addConstrainedSubviews([scrollView]) {
            scrollView.pinEdgesToSuperviewMargins(.all(configuration.layoutMargins))

scrollView.pinEdgesToSuperviewMargins(.all(configuration.layoutMargins)) will fix the last layout margin inconsistency in compact mode.

@buggmagnet buggmagnet force-pushed the nonconformant-design-voucher-submission-dialog-in-new-ios-311 branch from b6693f6 to 4d76e35 Compare September 21, 2023 08:50
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 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

@buggmagnet buggmagnet force-pushed the nonconformant-design-voucher-submission-dialog-in-new-ios-311 branch from feb1f52 to 55263e1 Compare September 21, 2023 09:54
@buggmagnet buggmagnet merged commit db1ffbb into main Sep 21, 2023
4 checks passed
@buggmagnet buggmagnet deleted the nonconformant-design-voucher-submission-dialog-in-new-ios-311 branch September 21, 2023 09:54
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.

2 participants