-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix bug 190: Brightness & Volume gestures only work when controls are… #200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leaved some review comments. Please fix them and I am going to build your branch to verify your changes.
feature/channel/src/main/java/com/m3u/feature/channel/ChannelMask.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/ChannelScreen.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/ChannelScreen.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/ChannelScreen.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/components/MaskGestureValuePanel.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/components/MaskGestureValuePanel.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/components/MaskGestureValuePanel.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/components/MaskGestureValuePanel.kt
Outdated
Show resolved
Hide resolved
feature/channel/src/main/java/com/m3u/feature/channel/components/MaskGestureValuePanel.kt
Outdated
Show resolved
Hide resolved
) | ||
Spacer(modifier = Modifier.width(4.dp)) | ||
MonoText( | ||
text = value, color = Color.White, fontSize = 20.sp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the "surface" color because its container is themed by onSurface color. Or just warp the whole component with a Surface and add colors params, it will pass the contentColor automatically, so that we needn't pass the textColor and iconTint by ourselves. I prefer the second way.
horizontalArrangement = Arrangement.SpaceBetween, | ||
verticalAlignment = Alignment.CenterVertically | ||
) { | ||
Image( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use Icon component.
) | ||
Spacer(modifier = Modifier.width(4.dp)) | ||
MonoText( | ||
text = value, color = Color.White, fontSize = 20.sp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The text size can be 14sp. It looks nicer~
verticalAlignment = Alignment.CenterVertically | ||
) { | ||
Image( | ||
imageVector = icon, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use LocalSpacing.current.small so that it will be scaled if user want to enable compact mode in settings.
The touch areas are somewhat confusing.
There are overlapping areas, making it difficult for users to control volume/brightness or dismiss the mask. |
Do you mean that the users are confused because the volume/brightness control area (green area) overlaps with the mask visibility toggle area (red area)? Shouldn't the mask only appear when users tap in the center of the screen, while swiping on the sides should adjust brightness and volume without showing the mask? And when swiping, users don't want the mask to appear and disrupt their experience, right? |
The user should be able to tap to show or hide the controls and swipe to adjust the volume and brightness, regardless of whether the controls are visible. Ensure some areas of the top layer remain empty to allow tap events to reach the bottom layer, and there’s no need to restrict the width of the bottom layer. |
And in other hands, the controls layer in the front of the gesture layer has also scroll event need to be consume in the center so that we can enter the EPG panel.
|
@oxyroid I have just pushed the new commit. Could you pls check it for me pls? Thanks very much for these comments ❤ |
Fix bug 190: Brightness & Volume gestures only work when controls are are displayed