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

Animate expansion of connection details view #7548

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SteffenErn
Copy link
Contributor

@SteffenErn SteffenErn commented Jan 30, 2025

It is finally time for some fancy animations of the connection view. A few changes to the structure, lots of pain and finally the salvation through .transformEffect(.identity) was required.
Some of the newly introduced State variables seem unnecessary, but it doesn't work properly without them.
Enjoy!


This change is Reviewable

Copy link

linear bot commented Jan 30, 2025

@SteffenErn SteffenErn force-pushed the animate-expansion-of-connection-details-ios-994 branch from bb3c5ae to 96ea06d Compare January 30, 2025 08:12
@SteffenErn SteffenErn added the iOS Issues related to iOS label Jan 30, 2025
@SteffenErn SteffenErn self-assigned this Jan 30, 2025
@SteffenErn SteffenErn requested a review from waahlnaden January 30, 2025 08:53
@SteffenErn SteffenErn force-pushed the animate-expansion-of-connection-details-ios-994 branch from 2bc4ae5 to f113e11 Compare January 30, 2025 11:26
This reverts commit f113e11.
This solution results in black bars on iOS version <18
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.

There is a black backround behind the buttons that should not be there:
Simulator Screenshot - iPhone 16 Pro - 2025-01-30 at 13.14.35.png

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @SteffenErn)


ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionView.swift line 18 at r3 (raw file):

    @State private(set) var showConnectionDetailsAnimated = false
    @State private(set) var isExpandedAnimatied = false

Nit: Spelling: "Animatied"

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: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @SteffenErn)


ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionView.swift line 17 at r3 (raw file):

    @State private(set) var isExpanded = false

    @State private(set) var showConnectionDetailsAnimated = false

I think we always want to do these things animated, so we don't need to specify that in the variable names.

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 10 files at r1, 6 of 7 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SteffenErn)


ios/MullvadVPN/Extensions/View+Modifier.swift line 12 at r4 (raw file):

extension View {
    func apply<V: View>(@ViewBuilder _ block: (Self) -> V) -> V { block(self) }

We should add documentation here to clarify what the function is used for.

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: all files reviewed, 4 unresolved discussions (waiting on @SteffenErn)


ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionView.swift line 18 at r4 (raw file):

    @State private(set) var showConnectionDetailsAnimated = false
    @State private(set) var isExpandedAnimatied = false

typo : isExpandedAnimated


ios/MullvadVPN/View controllers/Tunnel/ConnectionView/ConnectionView.swift line 40 at r4 (raw file):

        .background(BlurView(style: .dark))
        .cornerRadius(12)
        .padding(EdgeInsets(top: 16, leading: 16, bottom: 24, trailing: 16))

why did you remove constants?

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 3 of 10 files at r1, 6 of 7 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SteffenErn)

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