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

Paint status bar after screen enter animation end #5003

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

sabercodic
Copy link
Contributor

@sabercodic sabercodic commented Aug 10, 2023

Color phone status bar after fragment transition in following screens:

  • select location screen
  • account screen
  • settings screen

This change is Reviewable

@sabercodic sabercodic added the Android Issues related to Android label Aug 10, 2023
@linear
Copy link

linear bot commented Aug 10, 2023

DROID-216 Paint status bars after transition

This change order should be apply for following screens:

  • Account Screen
  • Setting Screen
  • Select Location Screen

Copy link
Contributor

@Pururun Pururun 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 r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 54 at r1 (raw file):

        }
    }
    LaunchedEffect(isCollapsable) {

I would use Unit instead.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt line 78 at r1 (raw file):

        }
    }
    LaunchedEffect(isCollapsable) {

I would use Unit instead.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 75 at r1 (raw file):

) {
    val context = LocalContext.current
    val isCollapsable by remember { mutableStateOf(false) }

This could be removed.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 80 at r1 (raw file):

    LaunchedEffect(Unit) { uiCloseAction.collect { onBackClick() } }
    LaunchedEffect(isCollapsable) {

Could be replaced with Unit.

@albin-mullvad
Copy link
Collaborator

Please clarify the PR title. It would also be nice with a brief description.

@sabercodic sabercodic changed the title Change paint statusbar time Paint status bar after screen enter animation end Aug 23, 2023
@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch 2 times, most recently from 284388a to 978c434 Compare August 23, 2023 12:29
Copy link
Contributor Author

@sabercodic sabercodic 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: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 54 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I would use Unit instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/Scaffolding.kt line 78 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I would use Unit instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 75 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This could be removed.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt line 80 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Could be replaced with Unit.

Done.

Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @Pururun)

Copy link
Collaborator

@albin-mullvad albin-mullvad 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 3 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @Pururun and @sabercodic)


-- commits line 2 at r2:
Clarify this commit message

Code quote:

Change paint statusbar time

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 55 at r2 (raw file):

    }
    LaunchedEffect(Unit) {
        delay(context.resources.getInteger(R.integer.transition_animation_duration).toLong())

I believe we should use a constant instead since it's easier to use in compose and we're moving away from resources for most other similar things (font styling, colors etc).

Code quote:

R.integer.transition_animation_duration

Copy link
Contributor Author

@sabercodic sabercodic 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: 2 of 4 files reviewed, 6 unresolved discussions (waiting on @albin-mullvad and @Pururun)


-- commits line 2 at r2:

Previously, albin-mullvad wrote…

Clarify this commit message

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 55 at r2 (raw file):

Previously, albin-mullvad wrote…

I believe we should use a constant instead since it's easier to use in compose and we're moving away from resources for most other similar things (font styling, colors etc).

Done, I

@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch from 978c434 to f45e268 Compare August 23, 2023 13:58
Copy link
Contributor

@Pururun Pururun 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 3 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)

Copy link
Collaborator

@albin-mullvad albin-mullvad 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 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 55 at r3 (raw file):

    }
    LaunchedEffect(Unit) {
        delay(SCREEN_ENTER_TRANSITION)

Is there no way of triggering a callback when the animation/transition is complete rather using a delay? It seems like that would make more sense, especially for cases such as disabled animations (in system settings).

If having a proper callback is simplified by using Compose Navigation and the animation/transition support added in 2.7.0 it might be worth skipping this for now.

Code quote:

delay

@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch from f45e268 to 4edbe82 Compare August 29, 2023 11:50
Copy link
Contributor Author

@sabercodic sabercodic 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 13 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/CollapsingToolbarScaffold.kt line 55 at r3 (raw file):

Previously, albin-mullvad wrote…

Is there no way of triggering a callback when the animation/transition is complete rather using a delay? It seems like that would make more sense, especially for cases such as disabled animations (in system settings).

If having a proper callback is simplified by using Compose Navigation and the animation/transition support added in 2.7.0 it might be worth skipping this for now.

Done.

@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch 2 times, most recently from f5a17ba to b129f9b Compare August 29, 2023 12:27
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch 3 times, most recently from 900f6a2 to ef19f56 Compare September 7, 2023 09:16
@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch from ef19f56 to 6022370 Compare September 11, 2023 11:55
Copy link
Collaborator

@albin-mullvad albin-mullvad 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 13 of 16 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sabercodic sabercodic force-pushed the paint-status-bars-after-transition-droid-216 branch from 6022370 to 438d66c Compare September 13, 2023 14:53
Copy link
Collaborator

@albin-mullvad albin-mullvad 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 1 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad force-pushed the paint-status-bars-after-transition-droid-216 branch from 438d66c to d67ccfb Compare September 13, 2023 15:24
@albin-mullvad albin-mullvad merged commit 40297a8 into main Sep 13, 2023
9 checks passed
@albin-mullvad albin-mullvad deleted the paint-status-bars-after-transition-droid-216 branch September 13, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants