-
Notifications
You must be signed in to change notification settings - Fork 200
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
Fix window insets to avoid overlapping display cutouts and system bars #1006
Fix window insets to avoid overlapping display cutouts and system bars #1006
Conversation
.asPaddingValues() | ||
.calculateEndPadding(layoutDirection), | ||
), | ||
modifier = Modifier.fillMaxSize(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -131,6 +133,11 @@ fun EventMapScreen( | |||
AnimatedTextTopAppBar( | |||
title = stringResource(EventMapRes.string.eventmap), | |||
scrollBehavior = scrollBehavior, | |||
windowInsets = WindowInsets( | |||
left = contentPadding.calculateLeftPadding(layoutDirection), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the amazing PR! I'll merge it regardless. However, could you explain why the window insets of each screen's AnimatedTextTopAppBar are different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takahirom
I gave this window insets to avoid display cutouts and system bars, since default value TopAppBarDefaults.windowInsets
only avoids system bars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explain. How about making a function like this? And can you resolve the conflict with the main branch?
windowInsets = AnimatedTextTopAppBarDefaults.windowInsets(contentPadding),
object AnimatedTextTopAppBarDefaults {
@Composable
fun windowInsets(contentPadding: PaddingValues): WindowInsets {
val layoutDirection = LocalLayoutDirection.current
return WindowInsets(
left = contentPadding.calculateLeftPadding(layoutDirection),
top = contentPadding.calculateTopPadding(),
right = contentPadding.calculateRightPadding(layoutDirection),
)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@takahirom
I realized I didn't need to use content padding for window insets of top app bars.
I'll make default windowInsets like below for each top app bars instead.
object HogeTopAppBarDefaults {
val windowInsets = WindowInsets.displayCutout.union(WindowInsets.systemBars).only(
WindowInsetsSides.Horizontal + WindowInsetsSides.Top,
)
}
…_screen_padding # Conflicts: # feature/contributors/src/commonMain/kotlin/io/github/droidkaigi/confsched/contributors/ContributorsScreen.kt # feature/main/src/commonMain/kotlin/io/github/droidkaigi/confsched/main/MainScreen.kt # feature/sponsors/src/commonMain/kotlin/io/github/droidkaigi/confsched/sponsors/SponsorsScreen.kt # feature/staff/src/commonMain/kotlin/io/github/droidkaigi/confsched/staff/StaffScreen.kt
Detekt check failed. Please run |
@norihirosunada Sorry for keeping you disturbed, but could you resolve the conflict? 🙇 |
…_screen_padding # Conflicts: # feature/main/src/commonMain/kotlin/io/github/droidkaigi/confsched/main/MainScreen.kt
@takahirom |
Let's merge! (Sorry, I've just released. I'm not sure if I can release this by time) |
Anyway Thank you for your contribution! |
Issue
Overview (Required)
Screenshot (Optional if screenshot test is present or unrelated to UI)