Fix broken auto styling #815
Conversation
auth/src/main/res/values/colors.xml
Outdated
| <color name="fui_colorPrimaryDark">#303F9F</color> | ||
| <color name="fui_colorAccent">#FF4081</color> | ||
| <resources xmlns:tools="http://schemas.android.com/tools"> | ||
| <color name="colorPrimary" tools:ignore="ResourceName">#3F51B5</color> |
There was a problem hiding this comment.
Oops. @TheCraftKid tried to stop me from doing this and I told him it would be fine.
Does this come into play if the app including FirebaseUI does not use AppCompat at all so colorPrimary is undefined?
There was a problem hiding this comment.
@samtstern oh sweet!!! You gave me an idea, I'll update this PR in just a few.
Signed-off-by: Alex Saveau <[email protected]>
| <item name="colorAccent">@color/fui_colorAccent</item> | ||
| <item name="colorPrimary">@color/colorPrimary</item> | ||
| <item name="colorPrimaryDark">@color/colorPrimaryDark</item> | ||
| <item name="colorAccent">@color/colorAccent</item> |
There was a problem hiding this comment.
@samtstern The reason we defined those colors was so that we could use them here. However, we are expecting them to be overridden so we don't actually need them to be fully defined colors, but my knowledge of the Android resource system wasn't that great back then. Anyway, now we just define them as ids of type color so the build will properly fail if devs using FirebaseUI don't define their colors and they'll be properly overridden if they are defined.
There was a problem hiding this comment.
Is the intended effect to require developers to override colorPrimary and etc.? I suppose that will work given we're recommending developers to use the AppCompat libraries.
There was a problem hiding this comment.
@TheCraftKid Yeah, since FirebaseUI v2.0.0 they already do, but this got broken in #796
There was a problem hiding this comment.
See #696 (comment) for an in-depth explanation.
auth/src/main/res/values/colors.xml
Outdated
| @@ -1,9 +1,5 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <resources> | |||
| <color name="fui_colorPrimary">#3F51B5</color> | |||
There was a problem hiding this comment.
Why are we now deleting these entirely rather than just removing the fui_?
There was a problem hiding this comment.
See my earlier comment. Those colors are basically a lie and we aren't expecting them to ever be used. The dev should have to override them so that FirebaseUI is styled correctly. In theory, 99% of people won't have to do anything because they'll already have those colors defined since AppComat uses them.
If you want, we can keep the defined colors but it makes it less clear to the developer that they need to override them because they won't get a build error. Your call! 😀
There was a problem hiding this comment.
Since it's technically a "breaking change" to remove them and cause a build error for that 1%, let's keep the default colors in place.
In a future version we can really pull the rug out and make sure developers override them.
There was a problem hiding this comment.
Ah, true. SGTM!
|
@SUPERCILEX this is the last thing I want to fix and then we can get out |
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
|
@samtstern Green! 😄 |
|
@SUPERCILEX thanks for being so helpful/responsive on this one. |
|
@SUPERCILEX thanks for being so helpful/responsive on this one. |
@samtstern Found some more broken res refactors.