Update Uamp Player Screen bottom buttons layout #2407
Conversation
|
|
||
| SettingsButtonsDefaults.BrandIcon( | ||
| modifier = Modifier | ||
| .align(Alignment.Bottom) | ||
| .weight(1f) |
|
Can we make sure this is covered by some sort of screenshot test? or is it already and there is no layout change? |
rajat4064g
force-pushed
the
uamp
branch
2 times, most recently
from
5d4b99a
to
9b687b8
Compare
| import org.robolectric.annotation.Config | ||
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
| class UampMediaPlayerA11yScreenshotTest : WearLegacyA11yTest() { |
There was a problem hiding this comment.
Why A11y test? Shoudl it just be a normal screenshot test?
|
This seems like a bigger change now, is it worth changing the title? |
|
Closing and reopening to trigger a build. |
There was a problem hiding this comment.
Is there an empty text here? Is that deliberate?
| textAlign = textAlign, | ||
| followGap = followGap, | ||
| startGap = startGap, | ||
| startEdgeGradientWidth = edgeGradientWidth, |
There was a problem hiding this comment.
Should this also set endEdgeGradientWidth?
| @ExperimentalHorologistApi | ||
| @Composable | ||
| public fun MarqueeText( | ||
| text: String, |
There was a problem hiding this comment.
We have a lot of new forms of this method, if this is a more advanced form and we want to keep the existing simpler forms, should we only add the AnnotatedString version?
| method @androidx.compose.runtime.Composable @com.google.android.horologist.annotations.ExperimentalHorologistApi public static void MarqueeText(String text, optional androidx.compose.ui.Modifier modifier, optional long color, optional androidx.compose.ui.text.TextStyle style, optional int textAlign, optional float followGap, optional float edgeGradientWidth, optional float marqueeDpPerSecond, optional long pauseTime); | ||
| method @androidx.compose.runtime.Composable @com.google.android.horologist.annotations.ExperimentalHorologistApi public static void MarqueeText(androidx.compose.ui.text.AnnotatedString text, optional androidx.compose.ui.Modifier modifier, optional java.util.Map<java.lang.String,androidx.compose.foundation.text.InlineTextContent> inlineContent, optional long color, optional androidx.compose.ui.text.TextStyle style, optional int textAlign, optional float followGap, optional float startGap, optional float startEdgeGradientWidth, optional float endEdgeGradientWidth, optional float marqueeDpPerSecond, optional long pauseTime); | ||
| method @androidx.compose.runtime.Composable @com.google.android.horologist.annotations.ExperimentalHorologistApi public static void MarqueeText(androidx.compose.ui.text.AnnotatedString text, optional androidx.compose.ui.Modifier modifier, optional java.util.Map<java.lang.String,androidx.compose.foundation.text.InlineTextContent> inlineContent, optional long color, optional androidx.compose.ui.text.TextStyle style, optional int textAlign, optional float followGap, optional float startGap, optional float edgeGradientWidth, optional float marqueeDpPerSecond, optional long pauseTime); | ||
| method @androidx.compose.runtime.Composable @com.google.android.horologist.annotations.ExperimentalHorologistApi public static void MarqueeText(String text, optional androidx.compose.ui.Modifier modifier, optional long color, optional androidx.compose.ui.text.TextStyle style, optional int textAlign, optional float followGap, optional float startGap, optional float startEdgeGradientWidth, optional float endEdgeGradientWidth, optional float marqueeDpPerSecond, optional long pauseTime); |
There was a problem hiding this comment.
If we think the new form with separate startGap, startEdgeGradientWidth, endEdgeGradientWidth is the right general form, then we should either
a) Remove the old form.
b) Leave them for compatibility, maybe deprecated.
As in the old methods are gone, since you have changed the params, added startGap.
| WearDeviceScreenshotTest(device = device) { | ||
|
|
||
| @Test | ||
| fun nothingPlayerScreen() = runTest { |
There was a problem hiding this comment.
Is it worth combining this into the test above? Just different test methods, both part of UampMediaPlayerScreenshotTest?
There was a problem hiding this comment.
Is this dangerously close to clipping? intended?
| ) | ||
| horizontalArrangement = Arrangement.Center, | ||
| ) { | ||
| Text( |
There was a problem hiding this comment.
Does this need to be Text? or can now just be an Icon?
| ) | ||
| MarqueeText( | ||
| text = currentTitle.orEmpty(), | ||
| startGap = if (titleIcon != null) 8.dp else 0.dp, |
There was a problem hiding this comment.
Does startGap really need to be part of MarqueeText? or could it be a Spacer in this Row?
There was a problem hiding this comment.
Are we sure this startGap is really needed? is this not just the startEdgeGradientWidth?
WHAT
Update Uamp Player Screen for two buttons and app icon layout at the bottom
WHY
HOW