From 9fd47261c96a0fe8d5f28e030b2f9cdc25b15895 Mon Sep 17 00:00:00 2001 From: Nick Rout Date: Thu, 15 Dec 2022 15:46:41 +0200 Subject: [PATCH] Address design system PR feedback --- .../core/designsystem/ThemeTest.kt | 148 ++++++++---------- .../core/designsystem/component/Button.kt | 4 +- .../designsystem/component/DropdownMenu.kt | 4 +- .../core/designsystem/component/ViewToggle.kt | 4 +- .../core/designsystem/theme/Theme.kt | 8 +- 5 files changed, 72 insertions(+), 96 deletions(-) diff --git a/core/designsystem/src/androidTest/java/com/google/samples/apps/nowinandroid/core/designsystem/ThemeTest.kt b/core/designsystem/src/androidTest/java/com/google/samples/apps/nowinandroid/core/designsystem/ThemeTest.kt index be53914f5..e97762318 100644 --- a/core/designsystem/src/androidTest/java/com/google/samples/apps/nowinandroid/core/designsystem/ThemeTest.kt +++ b/core/designsystem/src/androidTest/java/com/google/samples/apps/nowinandroid/core/designsystem/ThemeTest.kt @@ -21,6 +21,7 @@ import androidx.compose.material3.ColorScheme import androidx.compose.material3.MaterialTheme import androidx.compose.material3.dynamicDarkColorScheme import androidx.compose.material3.dynamicLightColorScheme +import androidx.compose.runtime.Composable import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.unit.dp @@ -62,16 +63,9 @@ class ThemeTest { ) { val colorScheme = LightDefaultColorScheme assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) + val gradientColors = defaultGradientColors(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = BackgroundTheme( - color = colorScheme.surface, - tonalElevation = 2.dp - ) + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } @@ -87,16 +81,9 @@ class ThemeTest { ) { val colorScheme = DarkDefaultColorScheme assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) + val gradientColors = defaultGradientColors(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = BackgroundTheme( - color = colorScheme.surface, - tonalElevation = 2.dp - ) + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } @@ -109,26 +96,11 @@ class ThemeTest { darkTheme = false, androidTheme = false ) { - val colorScheme = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - dynamicLightColorScheme(LocalContext.current) - } else { - LightDefaultColorScheme - } + val colorScheme = dynamicLightColorSchemeWithFallback() assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - GradientColors() - } else { - GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) - } + val gradientColors = dynamicGradientColorsWithFallback(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = BackgroundTheme( - color = colorScheme.surface, - tonalElevation = 2.dp - ) + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } @@ -141,26 +113,11 @@ class ThemeTest { darkTheme = true, androidTheme = false ) { - val colorScheme = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - dynamicDarkColorScheme(LocalContext.current) - } else { - DarkDefaultColorScheme - } + val colorScheme = dynamicDarkColorSchemeWithFallback() assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - GradientColors() - } else { - GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) - } + val gradientColors = dynamicGradientColorsWithFallback(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = BackgroundTheme( - color = colorScheme.surface, - tonalElevation = 2.dp - ) + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } @@ -176,7 +133,7 @@ class ThemeTest { ) { val colorScheme = LightAndroidColorScheme assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = GradientColors() + val gradientColors = emptyGradientColors assertEquals(gradientColors, LocalGradientColors.current) val backgroundTheme = LightAndroidBackgroundTheme assertEquals(backgroundTheme, LocalBackgroundTheme.current) @@ -194,7 +151,7 @@ class ThemeTest { ) { val colorScheme = DarkAndroidColorScheme assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = GradientColors() + val gradientColors = emptyGradientColors assertEquals(gradientColors, LocalGradientColors.current) val backgroundTheme = DarkAndroidBackgroundTheme assertEquals(backgroundTheme, LocalBackgroundTheme.current) @@ -209,23 +166,11 @@ class ThemeTest { darkTheme = false, androidTheme = true ) { - val colorScheme = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - dynamicLightColorScheme(LocalContext.current) - } else { - LightDefaultColorScheme - } + val colorScheme = dynamicLightColorSchemeWithFallback() assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - GradientColors() - } else { - GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) - } + val gradientColors = dynamicGradientColorsWithFallback(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = LightAndroidBackgroundTheme + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } @@ -238,28 +183,59 @@ class ThemeTest { darkTheme = true, androidTheme = true ) { - val colorScheme = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - dynamicDarkColorScheme(LocalContext.current) - } else { - DarkDefaultColorScheme - } + val colorScheme = dynamicDarkColorSchemeWithFallback() assertColorSchemesEqual(colorScheme, MaterialTheme.colorScheme) - val gradientColors = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { - GradientColors() - } else { - GradientColors( - top = colorScheme.inverseOnSurface, - bottom = colorScheme.primaryContainer, - container = colorScheme.surface - ) - } + val gradientColors = dynamicGradientColorsWithFallback(colorScheme) assertEquals(gradientColors, LocalGradientColors.current) - val backgroundTheme = DarkAndroidBackgroundTheme + val backgroundTheme = defaultBackgroundTheme(colorScheme) assertEquals(backgroundTheme, LocalBackgroundTheme.current) } } } + @Composable + private fun dynamicLightColorSchemeWithFallback(): ColorScheme { + return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { + dynamicLightColorScheme(LocalContext.current) + } else { + LightDefaultColorScheme + } + } + + @Composable + private fun dynamicDarkColorSchemeWithFallback(): ColorScheme { + return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { + dynamicDarkColorScheme(LocalContext.current) + } else { + DarkDefaultColorScheme + } + } + + private val emptyGradientColors = GradientColors() + + private fun defaultGradientColors(colorScheme: ColorScheme): GradientColors { + return GradientColors( + top = colorScheme.inverseOnSurface, + bottom = colorScheme.primaryContainer, + container = colorScheme.surface + ) + } + + private fun dynamicGradientColorsWithFallback(colorScheme: ColorScheme): GradientColors { + return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { + emptyGradientColors + } else { + defaultGradientColors(colorScheme) + } + } + + private fun defaultBackgroundTheme(colorScheme: ColorScheme): BackgroundTheme { + return BackgroundTheme( + color = colorScheme.surface, + tonalElevation = 2.dp + ) + } + /** * Workaround for the fact that the NiA design system specify all color scheme values. */ diff --git a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/Button.kt b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/Button.kt index 49dac704c..cab6fcd2f 100644 --- a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/Button.kt +++ b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/Button.kt @@ -234,12 +234,12 @@ fun NiaTextButton( * Internal Now in Android button content layout for arranging the text label and leading icon. * * @param text The button text label content. - * @param leadingIcon The button leading icon content. Pass `null` here for no leading icon. + * @param leadingIcon The button leading icon content. Default is `null` for no leading icon.Ï */ @Composable private fun NiaButtonContent( text: @Composable () -> Unit, - leadingIcon: @Composable (() -> Unit)? + leadingIcon: @Composable (() -> Unit)? = null ) { if (leadingIcon != null) { Box(Modifier.sizeIn(maxHeight = ButtonDefaults.IconSize)) { diff --git a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/DropdownMenu.kt b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/DropdownMenu.kt index 3aefaa9be..b8d3fa837 100644 --- a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/DropdownMenu.kt +++ b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/DropdownMenu.kt @@ -115,12 +115,12 @@ fun NiaDropdownMenuButton( * trailing icon. * * @param text The button text label content. - * @param trailingIcon The button trailing icon content. Pass `null` here for no trailing icon. + * @param trailingIcon The button trailing icon content. Default is `null` for no trailing icon. */ @Composable private fun NiaDropdownMenuButtonContent( text: @Composable () -> Unit, - trailingIcon: @Composable (() -> Unit)?, + trailingIcon: @Composable (() -> Unit)? = null, ) { Box( Modifier diff --git a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/ViewToggle.kt b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/ViewToggle.kt index 02ee20512..a6721c78c 100644 --- a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/ViewToggle.kt +++ b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/component/ViewToggle.kt @@ -77,12 +77,12 @@ fun NiaViewToggleButton( * trailing icon. * * @param text The button text label content. - * @param trailingIcon The button trailing icon content. Pass `null` here for no trailing icon. + * @param trailingIcon The button trailing icon content. Default is `null` for no trailing icon. */ @Composable private fun NiaViewToggleButtonContent( text: @Composable () -> Unit, - trailingIcon: @Composable (() -> Unit)?, + trailingIcon: @Composable (() -> Unit)? = null, ) { Box( Modifier diff --git a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/theme/Theme.kt b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/theme/Theme.kt index 03d214dcc..80b635be0 100644 --- a/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/theme/Theme.kt +++ b/core/designsystem/src/main/java/com/google/samples/apps/nowinandroid/core/designsystem/theme/Theme.kt @@ -241,10 +241,10 @@ internal fun NiaTheme( color = colorScheme.surface, tonalElevation = 2.dp ) - val backgroundTheme = if (androidTheme) { - if (darkTheme) DarkAndroidBackgroundTheme else LightAndroidBackgroundTheme - } else { - defaultBackgroundTheme + val backgroundTheme = when { + !disableDynamicTheming -> defaultBackgroundTheme + androidTheme -> if (darkTheme) DarkAndroidBackgroundTheme else LightAndroidBackgroundTheme + else -> defaultBackgroundTheme } // Composition locals CompositionLocalProvider(