fix(auth): allow customizing top app bar colors via AuthUITheme#2391
fix(auth): allow customizing top app bar colors via AuthUITheme#2391demolaf wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to customize top app bar colors on authentication screens by adding a topAppBarColors property to AuthUITheme and applying it across various UI screens. The review feedback highlights a compilation error in HighLevelApiDemoActivity.kt due to the use of a non-existent subtitleContentColor parameter in Material 3's TopAppBarColors. Additionally, because TopAppBarColors is an experimental API, the reviewer recommends adding the @OptIn(ExperimentalMaterial3Api::class) annotation locally in the demo activity and propagating the @property:ExperimentalMaterial3Api annotation to the public property in AuthUITheme.
russellwheatley
left a comment
There was a problem hiding this comment.
A few bits of feedback
SelectFactorUI's TopAppBar in MfaEnrollmentDefaults.kt doesn't take colors at all, so it won't pick up topAppBarColors (or even the existing default). Can you wire that one up too, otherwise the 2FA screen won't match once someone sets a custom color.
see auth/src/main/java/com/firebase/ui/auth/ui/screens/MfaEnrollmentDefaults.kt:255 (the TopAppBar call inside SelectFactorUI)
| * Custom colors for the top app bar shown on auth screens. If null, falls back to | ||
| * colors derived from [colorScheme] (see [AuthUITheme.topAppBarColors]). | ||
| */ | ||
| val topAppBarColors: TopAppBarColors? = null, |
There was a problem hiding this comment.
LocalAuthUITheme.current.topAppBarColors ?: AuthUITheme.topAppBarColors is copied into all 6 screen files - can we pull that into one resolved property on AuthUITheme instead? Saves updating 6 files next time this logic changes.
|
|
||
| @OptIn(ExperimentalMaterial3Api::class) | ||
| @Test | ||
| fun `Copy with custom topAppBarColors applies correctly`() { |
There was a problem hiding this comment.
The new tests just re-check the theme object, not the actual screens - since the fallback's duplicated everywhere, this won't catch a screen missing it. Worth testing off the shared helper once it's extracted (see point 1)
Fixes #2390.
AuthUITheme.topAppBarColorswas a companion-object getter derived from the ambientMaterialTheme.colorScheme, so there was no way to override it perAuthUIThemeinstance — apps couldn't align the auth screens' top app bar with their own branding.Usage
Preview