Repository navigation
Conversation
…o the full customization demo
…rom their light palettes
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic theme-switching feature to the full customization demo, adding 'Doggo', 'Grid', and 'Coffee Spark' themes. It refactors several screens and components to use theme-aware styles, shapes, and backgrounds. The review feedback focuses on performance and UI polish: reading SharedPreferences outside of composition to avoid blocking the main thread, removing redundant background drawing and transparent scrims to prevent unnecessary overdraw, and clipping the theme switcher's selectable items to ensure the click ripple matches their rounded shape.
| val preferences = getSharedPreferences(THEME_PREFERENCES, MODE_PRIVATE) | ||
|
|
||
| setContent { | ||
| FullCustomizationTheme { | ||
| Surface( | ||
| modifier = Modifier.fillMaxSize(), | ||
| color = MaterialTheme.colorScheme.background | ||
| ) { | ||
| FirebaseAuthScreen( | ||
| configuration = configuration, | ||
| authUI = authUI, | ||
| emailLink = emailLink, | ||
| onSignInSuccess = { result -> | ||
| Log.d("FullCustomizationDemo", "Auth success: ${result.user?.uid}") | ||
| }, | ||
| onSignInFailure = { exception: AuthException -> | ||
| Log.e("FullCustomizationDemo", "Auth failed", exception) | ||
| }, | ||
| onSignInCancelled = { | ||
| Log.d("FullCustomizationDemo", "Auth cancelled") | ||
| }, | ||
| mfaConfiguration = MfaConfiguration( | ||
| allowedFactors = listOf(MfaFactor.Sms, MfaFactor.Totp), | ||
| requireEnrollment = false, | ||
| ), | ||
| customMethodPickerLayout = { providers, onProviderSelected -> | ||
| MainUI( | ||
| authUI = authUI, | ||
| configuration = configuration, | ||
| providers = providers, | ||
| onProviderSelected = onProviderSelected, | ||
| ) | ||
| }, | ||
| // Covers the email flows the library navigates to itself. | ||
| emailContent = { state -> EmailAuthUI(state) }, | ||
| phoneContent = { state -> PhoneSignInUI(state) }, | ||
| mfaEnrollmentContent = { state -> MfaEnrollmentUI(state) }, | ||
| mfaChallengeContent = { state -> MfaChallengeUI(state) }, | ||
| reauthContent = { state -> ReauthUI(state) }, | ||
| authenticatedContent = { state, uiContext -> | ||
| AuthenticatedUI(state = state, uiContext = uiContext) | ||
| var themeId by remember { | ||
| mutableStateOf( | ||
| FullCustomizationThemeId.entries.firstOrNull { | ||
| it.name == preferences.getString(THEME_KEY, null) | ||
| } ?: FullCustomizationThemeId.Doggo | ||
| ) | ||
| } |
There was a problem hiding this comment.
Reading SharedPreferences directly inside the remember block during composition can block the main thread and cause frame drops, as composition should be fast and side-effect free. It is highly recommended to read the initial theme value outside of the setContent block and pass it as the initial state.
val preferences = getSharedPreferences(THEME_PREFERENCES, MODE_PRIVATE)
val initialTheme = FullCustomizationThemeId.entries.firstOrNull {
it.name == preferences.getString(THEME_KEY, null)
} ?: FullCustomizationThemeId.Doggo
setContent {
var themeId by remember { mutableStateOf(initialTheme) }| ) | ||
| } | ||
| // The art is drawn for a light page; dark mode washes it back toward the dark one. | ||
| Box(modifier = Modifier.fillMaxSize().background(style.artScrim)) |
There was a problem hiding this comment.
When style.artScrim is Color.Transparent (which is the case for all light themes), drawing a full-screen Box with a transparent background is redundant and adds an unnecessary overdraw pass. Wrapping this in a conditional check to only draw the scrim when it is not transparent improves rendering efficiency.
if (style.artScrim != Color.Transparent) {
Box(modifier = Modifier.fillMaxSize().background(style.artScrim))
}| Box( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .background(MaterialTheme.colorScheme.background) | ||
| .clipToBounds(), | ||
| ) { |
There was a problem hiding this comment.
The root Surface in FullCustomizationDemoActivity already sets the background color to MaterialTheme.colorScheme.background. Drawing the background again in PlacedArt is redundant and causes unnecessary overdraw. Removing the .background(...) modifier improves rendering performance.
Box(
modifier = Modifier
.fillMaxSize()
.clipToBounds(),
) {| modifier = Modifier | ||
| .background( | ||
| color = if (isSelected) pill.content else Color.Transparent, | ||
| shape = RoundedCornerShape(percent = 50), | ||
| ) | ||
| .selectable( | ||
| selected = isSelected, | ||
| role = Role.RadioButton, | ||
| onClick = { onSelect(theme) }, | ||
| ) | ||
| .padding(horizontal = 12.dp, vertical = 6.dp), |
There was a problem hiding this comment.
The selectable items in the theme switcher have a rounded background shape (RoundedCornerShape(percent = 50)), but the click ripple indication is not clipped to this shape. This results in a rectangular ripple effect when clicked. Applying .clip(RoundedCornerShape(percent = 50)) before .selectable ensures the ripple matches the rounded background. Note that you will need to import androidx.compose.ui.draw.clip.
modifier = Modifier
.background(
color = if (isSelected) pill.content else Color.Transparent,
shape = RoundedCornerShape(percent = 50),
)
.clip(RoundedCornerShape(percent = 50))
.selectable(
selected = isSelected,
role = Role.RadioButton,
onClick = { onSelect(theme) },
)
.padding(horizontal = 12.dp, vertical = 6.dp),…themed CTA colours throughout
…layout to the full customization demo
demolaf
changed the title
Adds Grid, Coffee Spark and Professional themes to the full customization demo, a switcher on every screen, and dark variants of all four.
Changes
Preview