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

Add missing scroll bar to remaining screens #3895

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

dshokouhi
Copy link
Member

@dshokouhi dshokouhi commented Sep 27, 2023

Summary

We had a play store review failure. One of the failing items was missing scroll bars when a user is scrolling.

To fix this I am adding Scaffold to ThemeLazyColumn to ensure we always show the scroll bars and time

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

@jpelgrom
Copy link
Member

When adding a Scaffold anywhere we use a ThemeLazyColumn, doesn't it make more sense to create a new Composable with Scaffold + ThemeLazyColumn?

@dshokouhi
Copy link
Member Author

When adding a Scaffold anywhere we use a ThemeLazyColumn, doesn't it make more sense to create a new Composable with Scaffold + ThemeLazyColumn?

Honestly I thought about adding the Scaffold to the Lazy Column as we should always use the 2 together no matter what. In reality WearAppTheme + Scaffold + ThemeLazyColumn should always be used together right?

Not sure if we should combine them or just continue to keep it separate but IMO we use it as a theme to show the scroll bar and time in the lazy column while scrolling so the 3 feel very much related.

@jpelgrom
Copy link
Member

WearAppTheme is not always followed by a scaffold (eg navigation), but it's a function so you can create multiple and use them as needed :) Combining Scaffold + ThemeLazyColumn would make sense because they both use the same state for the scroll indicator.

@dshokouhi
Copy link
Member Author

Combining Scaffold + ThemeLazyColumn would make sense because they both use the same state for the scroll indicator.

completed in the latest commit

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

A general pattern for composables is:

val scalingLazyListState = rememberScalingLazyListState()
WearAppTheme {
    ThemeLazyColumn(state = scalingLazyListState) {
        ...
    }
}

But as the ThemeLazyColumn now includes a Scaffold, which was almost always the only reason to get the state in a variable, can't it be removed in a lot of places?

@dshokouhi
Copy link
Member Author

A general pattern for composables is:

val scalingLazyListState = rememberScalingLazyListState()
WearAppTheme {
    ThemeLazyColumn(state = scalingLazyListState) {
        ...
    }
}

But as the ThemeLazyColumn now includes a Scaffold, which was almost always the only reason to get the state in a variable, can't it be removed in a lot of places?

Do you mean to stick with the default which also gets the latest state? I think so but was used to always grabbing the state and sending it in :)

@jpelgrom
Copy link
Member

Do you mean to stick with the default which also gets the latest state? I think so but was used to always grabbing the state and sending it in :)

I think simply putting ThemeLazyColumn { ... } (using the default parameter) would work.

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Installed and tested, I think you got all screens now. Love how simple something like the sensor list is with these changes.

@JBassett JBassett merged commit a15f7b3 into home-assistant:master Sep 29, 2023
4 checks passed
@dshokouhi dshokouhi deleted the wear_missing_scrollbar branch October 1, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants