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 4 commits into
base: master
Choose a base branch
from

Conversation

manpreeeeeet
Copy link

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

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

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

PinSetupScreen and PinRemoveScreen to use
the small button size.
@manpreeeeeet
Copy link
Author

Latest changes

Small Phone:
image
image

Medium Phone:
image
image
image

Fold:
image
image
image
image

@@ -106,7 +110,8 @@ fun PinButton(

object PinButtonDefaults {

val PinButtonMinSize = 72.dp
val PinButtonSmallMinSize = 56.dp
Copy link
Author

Choose a reason for hiding this comment

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

My goal was to make this as big as possible while still fitting all the 3 screens i was testing.

}
}
)
if (orientation == Configuration.ORIENTATION_LANDSCAPE) {
Copy link
Author

Choose a reason for hiding this comment

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

Switch AppBar based on orientation.

},
description = null,
useSmallButtons = orientation == Configuration.ORIENTATION_LANDSCAPE,
Copy link
Author

@manpreeeeeet manpreeeeeet Jan 3, 2025

Choose a reason for hiding this comment

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

Use smaller buttons for these 2 screens.

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