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

(MVP): Add landscape support for the PIN layout. #74

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manpreeeeeet
Copy link

398021532-9024af2f-bf4b-4630-969c-7bea243c7fcb

.fillMaxHeight()
.padding(it)
.padding(40.dp)
.padding(bottom = 16.dp),
Copy link
Owner

Choose a reason for hiding this comment

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

It's better to merge these paddings into one modifier

Copy link
Author

Choose a reason for hiding this comment

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

Just so I am understanding this correctly, I do not see a visual difference when I remove padding(it) here, which is expected since we already do padding(40.dp). So we can safely remove this for the row layout, is that right?

Copy link
Owner

Choose a reason for hiding this comment

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

.padding(it) applies additional paddings provided by Scaffold to ensure the top and bottom bars don't overlap with the content. Removing it will also result in a compiler warning/error

val orientation = LocalConfiguration.current.orientation
if (orientation == Configuration.ORIENTATION_LANDSCAPE) {

FlowColumn(
Copy link
Owner

Choose a reason for hiding this comment

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

Any reasons why this is a FlowColumn and not a FlowRow?

Copy link
Author

Choose a reason for hiding this comment

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

I chose FlowColumn since in landscape fitting all the buttons requires reducing the height as low as possible rather than the width. And using FlowColumn + weight(1f) made it easier.

But now taking a second look at it, a fixed width for the Box that contains these buttons should be able to achieve this without having to do it using FlowColumns.

Copy link
Owner

Choose a reason for hiding this comment

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

The reason I asked was because FlowRow would allow extracting the content into a separate composable

}
}
Spacer(modifier = Modifier.weight(1f))
Box(modifier = Modifier.weight(0.4f)) {
Copy link
Author

Choose a reason for hiding this comment

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

Alternative to FlowColumn requires this code, where we need to manually calculate all the padding + minimumButton size. This does remove the duplicated content inside of FlowColumn and FlowRow but it does make it complex to read IMO, since it is not obvious what/how it is calculated without taking a closer look.

I am leaning towards the FlowColumn approach. Thoughts?

                val pinBoardHorizontalPadding = 32.dp 
                val horizontalSpacedBy = 16.dp 
                val totalPadding = (pinBoardHorizontalPadding * 2) + (horizontalSpacedBy * 2)
                val minimumBoxWidth = remember(PinButtonDefaults.PinButtonMinSize) {
                    val buttonsInRow = 3
                    (PinButtonDefaults.PinButtonMinSize * buttonsInRow) + totalPadding
                }
                Box(
                    modifier = Modifier.width(minimumBoxWidth),
                    contentAlignment = Alignment.Center
                ) {
                    PinBoard(
                        modifier = Modifier
                            .padding(horizontal = pinBoardHorizontalPadding)
                            .padding(bottom = 16.dp),
                        horizontalSpace = horizontalSpacedBy,
                        state = state
                    )
                }

Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd rather have this than duplicated code

fit 3 pin buttons.

In landscape mode, height is the limiting constraint. So, this calculation tries to
fit exactly 3 buttons in a row using their
`PinButtonDefaults.PinButtonMinSize`.
@manpreeeeeet
Copy link
Author

Note 10 Plus:
image

Medium Phone (Android Emulator):
image

Tablet:
image

Fold:
image

Fold but folded:
image

@manpreeeeeet
Copy link
Author

For small screens, the PinSetup and PinRemoval screens would require additional changes. This is what it looks like currently:
image

This is not a problem on tablets.

Switching the LargeTopBar with Topbar in landscape mode gets us this far:
image

So we need to reduce the MinimumButton size to make it fit. Reducing the min size to 60.dp:
image

@manpreeeeeet
Copy link
Author

Also a dumb question, when you refer to AOSP's style, how/where do you find the screens that you reference. Example:
image

@X1nto
Copy link
Owner

X1nto commented Dec 26, 2024

Also a dumb question, when you refer to AOSP's style, how/where do you find the screens that you reference.

I boot up the official emulator images for stuff like pin input. For more common layouts I use https://m3.material.io

@X1nto
Copy link
Owner

X1nto commented Jan 1, 2025

TopAppBar in landscape looks wonderful, let's do that to fit the buttons. Otherwise, this looks great. I'll make some final adjustments for larger screens on landscape after this

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.

2 participants