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

[SystemUiController] Update SystemUiController to hoist dark theme #1275

Merged
merged 1 commit into from Aug 3, 2022

Conversation

alexvanyo
Copy link
Collaborator

Reworks the primary docs entrance to the system ui controller module slightly.

First, it removes the reference to MaterialTheme.colors.isLight. This is M2 specific, and doesn't exist in M3 in favor of hoisting the decision directly.

Second, it replaces SideEffect with a DisposableEffect with keys. I could go either way here, but I'm less of a fan of the implicit nature in how SideEffect reruns. It's not super obvious that this wouldn't work when useDarkIcons changes:

val systemUiController = rememberSystemUiController()
var useDarkIcons by remember { mutableStateOf(false) }

SideEffect {
    systemUiController.setSystemBarsColor(
        color = Color.Transparent,
        darkIcons = useDarkIcons
    )
}

@bentrengrove
Copy link
Collaborator

SideEffect runs on every recomposition, why wouldn't your above example work?

@alexvanyo
Copy link
Collaborator Author

SideEffect runs on every recomposition, why wouldn't your above example work?

That's true, but SideEffect will only recompose once.
Since var useDarkIcons is using a delegate, no state reads happen in the outer scope, so the SideEffect won't be recompose when useDarkIcons changes. Unless there are other state reads of it later.

@alexvanyo alexvanyo merged commit e69b0bf into main Aug 3, 2022
@alexvanyo alexvanyo deleted the av/system-ui-controller-hoist-dark branch August 3, 2022 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants