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 Adaptive Layout Codelab edge-to-edge #485

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

Conversation

ashnohe
Copy link

@ashnohe ashnohe commented Aug 30, 2024

MainActivity.kt

  • Called enableEdgeToEdge()
  • Removed translucency applied to 3-button navigation bar with isNavigationBarContrastEnforced=false

Theme.kt

  • Removed all logic needed to set the status bar icon colors. Tested in both light & dark theme.

ReplyListContent.kt

  • Added a new function to make a copy of PaddingValues. Used in the next bullet.
  • Ensured first and last list items do not hide behind status bars, items scroll under the status bars, and excess bottom padding is removed in ReplyListPane and ReplyDetailPane. To do this, I set contentPadding = WindowInsets.safeDrawing.asPaddingValues().copy(layoutDirection, bottom = 0.dp).
  • Now that we're accounting for the top system bars, removed excess top padding by replacing padding(16.dp) with padding(horizontal = 16.dp).

These changes are made to the main branch.

Portrait:
Screenshot_20240829-155943

Landscape (looks a little weird, not yet sure what to do about this):
Screenshot_20240829-160014

Unfolded, showing the last list item:
Screenshot_20240829-162448

When this commit is cherry picked to the end branch the app will look like this:
Screenshot_20240829-163202

@ashnohe ashnohe requested a review from a team as a code owner August 30, 2024 00:01
@ashnohe ashnohe changed the title Add edge-to-edge for the Adaptive Layout Codelab Make the Adaptive Layout Codelab edge-to-edge Aug 30, 2024
Comment on lines +72 to +73
contentPadding = WindowInsets.safeDrawing.asPaddingValues()
.copy(layoutDirection, bottom = 0.dp)

Choose a reason for hiding this comment

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

I think this can be done with built-in APIs as

Suggested change
contentPadding = WindowInsets.safeDrawing.asPaddingValues()
.copy(layoutDirection, bottom = 0.dp)
contentPadding = WindowInsets.safeDrawing.only(WindowInsetsSides.Horizontal + WindowInsetsSides.Top).asPaddingValues()

Comment on lines +97 to +98
contentPadding = WindowInsets.safeDrawing.asPaddingValues()
.copy(layoutDirection = layoutDirection, bottom = 0.dp)

Choose a reason for hiding this comment

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

Ditto here

Suggested change
contentPadding = WindowInsets.safeDrawing.asPaddingValues()
.copy(layoutDirection = layoutDirection, bottom = 0.dp)
contentPadding = WindowInsets.safeDrawing.only(WindowInsetsSides.Horizontal + WindowInsetsSides.Top).asPaddingValues()

end: Dp? = null,
bottom: Dp? = null,
) = PaddingValues(
start = start ?: calculateStartPadding(layoutDirection),

Choose a reason for hiding this comment

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

I think this can be removed entirely with the alternate APIs above, but one subtle behavior issue that can arise with this code:

This causes calculateStartPadding and other calculate* methods to be invoked during composition. The correct inset values (as exposed in the PaddingValues) won't necessarily be correct during composition, so anything based on the values that are computed during composition can end up resulting in being off by one frame.

A solution to this here would be to setup a PaddingValues that defers calling the calculate* values on the source PaddingValues, so that they are read later when the inset values will be correct:

fun PaddingValues.copy(
    start: Dp? = null,
    top: Dp? = null,
    end: Dp? = null,
    bottom: Dp? = null,
) = object : PaddingValues {
    override fun calculateBottomPadding(): Dp =
        bottom ?: this@copy.calculateBottomPadding()

    override fun calculateLeftPadding(layoutDirection: LayoutDirection): Dp =
        when (layoutDirection) {
            LayoutDirection.Ltr -> start
            LayoutDirection.Rtl -> end
        } ?: this@copy.calculateLeftPadding(layoutDirection)

    override fun calculateRightPadding(layoutDirection: LayoutDirection): Dp =
        when (layoutDirection) {
            LayoutDirection.Ltr -> end
            LayoutDirection.Rtl -> start
        } ?: this@copy.calculateRightPadding(layoutDirection)

    override fun calculateTopPadding(): Dp =
        top ?: this@copy.calculateTopPadding()
}

Copy link
Contributor

@riggaroo riggaroo left a comment

Choose a reason for hiding this comment

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

I think Alex's comment should be addressed and then we good to merge!

@@ -34,6 +36,11 @@ class MainActivity : ComponentActivity() {
override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

enableEdgeToEdge()
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't seen this done before, should we add it to the compose docs for edge to edge?

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.

3 participants