google / horologist

Horologist is a group of libraries that aim to supplement Wear OS developers with features that are commonly required by developers but not yet available.
https://google.github.io/horologist/
Apache License 2.0
543 stars 87 forks source link

Don't show PositionIndicator for non scrollable dialog content #2265

Closed yschimke closed 2 weeks ago

yschimke commented 2 weeks ago

WHAT

Don't show PositionIndicator for non scrollable dialog content

WHY

should seem non scrollable

HOW

Checklist :clipboard:

garanj commented 2 weeks ago

Hey Yuri! Curious as to how non-scrollable dialogs work: Does the developer have to declare whether it is non-scrollable or not, or does Compose calculate whether scrolling is required? (apologies, I couldn't easily see the right bit)

yschimke commented 2 weeks ago

@garanj Developer has to specify. It will look poor, but still be scrollable if they get it wrong.

yschimke commented 2 weeks ago

@garanj will land based on discussion, care to LGTM?

Tolriq commented 2 weeks ago

@yschimke just tried to understand why it was like that while creating an M3 version and wonder why not choose an automtic solution such as :

    val canScroll by remember { derivedStateOf { state.canScrollForward || state.canScrollBackward } }
    ScreenScaffold(
        modifier = modifier.fillMaxSize(),
        scrollState = if (canScroll) state else null,
        timeText = {},
    ) {
yschimke commented 2 weeks ago

@Tolriq yeah - that's a valid question. It would work, and probably safer for potential rejections.

This component definitely isn't a clean solution, and needing a scrollbar indicates a poor layout and user experience.

In this matrix of screenshots, the grey row is why a Non Scrollable variant exists, otherwise it's too high on screen. So the previous fix changes that to the middle row. But unfortunately introduces the issue (too low on screen) in the orange row when you have too much content.

I don't have a better fix for this with the current Wear Compose APIs.

image

Tolriq commented 2 weeks ago

Hum yes, I also changed the scaling lazy to use fake item at top and bottom instead of vertical content padding to fix the indicator (including the M3 one) but also requires changes to the scroll away.

Will do more test and play with a local copy for now.

Thanks for the details.