Update README.md to include app_name and color requirements #862

WillieCubed · 2017-08-09T02:49:47Z

This adds some information that has been discussed in #696, #815 and others.

SUPERCILEX

@TheCraftKid Sorry for the deluge of comments, I'm a little OCD about README quality. 😄

SUPERCILEX · 2017-08-09T03:04:23Z

auth/README.md

+For example, something like this goes in your `strings.xml`:
+```xml
+<resources>
+    <!-- ... -->


nit: can we put the ... after app_name since the latter should in theory be first?

In the Android Studio template, app_name is first... Will do.

SUPERCILEX · 2017-08-09T03:05:33Z

auth/README.md

+
+This goes in your `styles.xml`:
+```xml
+<style name="AppTheme" parent="FirebaseUI">


The parent should be something like Theme.AppCompat.DayNight.DarkActionBar or Theme.AppCompat.Light.DarkActionBar, not FirebaseUI.

SUPERCILEX · 2017-08-09T03:06:52Z

auth/README.md

+    <!-- Required for sign-in flow styling -->
+    <item name="colorPrimary">@color/material_green_500</item>
+    <item name="colorPrimaryDark">@color/material_green_700</item>
+    <item name="colorAccent">@color/material_purple_a700</item>


The standard code Android Studio generates has the app style reference each color name like so instead of referencing the color directly:

<item name="colorPrimary">@color/colorPrimary</item> <item name="colorPrimaryDark">@color/colorPrimaryDark</item> <item name="colorAccent">@color/colorAccent</item>

Whoops, I knew I forgot to change something while copy-pasting.

SUPERCILEX · 2017-08-09T03:10:22Z

auth/README.md

+AppCompat color attributes in your app.
+
+For example, something like this goes in your `strings.xml`:
+```xml


nit: to follow the existing style in this README, we should have a space between all the ``` and their text.

SUPERCILEX · 2017-08-09T03:12:49Z

auth/README.md

+To use FirebaseUI Auth's sign-in flows, you must provide an `app_name` string and use the
+AppCompat color attributes in your app.
+
+For example, something like this goes in your `strings.xml`:


Could we improve the clarity of this statement to something like this?

First, ensure an `app_name` resource is defined your `strings.xml` file like so:

Yeah, phrasing this was a problem for me. That is way better.

SUPERCILEX · 2017-08-09T03:16:48Z

auth/README.md

+```
+
+This goes in your `styles.xml`:
+```xml


Could we improve the clarity of this statement to something like this?

Second, ensure the three standard AppCompat color resources are defined with your own values:

nit: add line break

SUPERCILEX · 2017-08-09T03:18:57Z

auth/README.md

-redefine a string to change it, for example:
+If you wish to change the string messages, the existing strings can be overridden
+by name in your application. See the module's [strings.xml](src/main/res/values/strings.xml) file
+and simply redefine a string to change it, for example:


We could simplify this to and simply redefine a string to change it:

SUPERCILEX · 2017-08-09T03:22:21Z

auth/README.md

 </resources>
 ```
+Note: String resource names are internal to the library and there are currently no
+guarantees that they won't change between updates.


This should be a little scarier: 👻😆

**Note:** string resource names aren't considered part of the public API and might therefore change and break your app between library updates. We recommend looking at a diff of the `strings.xml` file before updating FirebaseUI.

And I thought mine was too abrasive. 😆

@TheCraftKid haha, yeah. We've broken the strings without remorse several times now, so we want to make sure devs understand that we aren't kidding. 😄

SUPERCILEX · 2017-08-09T03:22:29Z

auth/README.md


 ```xml
 <resources>
    <!-- was "Signing up..." -->
-    <string name="progress_dialog_signing_up">Creating your shiny new account...</string>
+    <string name="fui_progress_dialog_signing_up">Creating your shiny new account...</string>


SUPERCILEX · 2017-08-09T03:35:35Z

auth/README.md

+Note: If your app is already using an AppCompat theme, you don't have to create 
+a new theme, but you can still optionally style other parts of the sign-in flow. 
+Standard material design colorand typography properties will take effect as expected. 
+For example, to define a green theme:


I think this paragraph up to You don't have to call... should be replaced with something simpler since we don't talk about setTheme anywhere else:

If you would like more control over FirebaseUI's styling, you can define your own custom style to override certain or all styling attributes. For example, a green sign-in theme:

PS: we don't talk about setTheme anywhere except here which we should remove.

Instead of removing the earlier mention completely, why don't we just provide a link to the lower section?

@TheCraftKid Hmmm, the problem I see with this section is that it feels kinda like an "upgrade guide" of sorts. I'm trying to think of it from a newcomer's perspective, and for all they care, we've always auto-styled FirebaseUI so why would they have to call setTheme()? If you believe a newcomer would find this information useful, then feel free to keep it around.

samtstern · 2017-08-09T15:43:53Z

I just want to say I love seeing a PR where I am neither the author nor the reviewer! I'll merge this once you two have worked through the comments.

WillieCubed · 2017-08-09T17:16:41Z

Hey, do we still require the Fabric Maven repo in the build.gradle for FirebaseUI Auth?

samtstern · 2017-08-09T17:25:08Z

@TheCraftKid we only require it if you're using Twitter auth, since you need it to resolve the twitter dependency.

SUPERCILEX

@TheCraftKid Two last comments, everything else LGTM! 😄

SUPERCILEX · 2017-08-09T20:10:10Z

auth/README.md

 </resources>
 ```
+**Note:** String resource names aren't considered part of the public API and might


nit: add space between text and ```.

SUPERCILEX · 2017-08-09T20:12:47Z

auth/README.md

+startActivityForResult(
+    AuthUI.getInstance(this).createSignInIntentBuilder()
+        .build());
+```


Are you sure you want to keep this section?

SUPERCILEX

@TheCraftKid awesome! 😀👍

@samtstern LGTM!

Update README.md to include app_name and color requirements

8b8707c

SUPERCILEX reviewed Aug 9, 2017

View reviewed changes

WillieCubed added 2 commits August 9, 2017 15:05

Add clarifying statements and update information

db4c8d1

Fix typo

b4b8f6b

SUPERCILEX reviewed Aug 9, 2017

View reviewed changes

Add more last minute fixes

d2ef388

SUPERCILEX approved these changes Aug 10, 2017

View reviewed changes

samtstern merged commit 49ca5e1 into firebase:master Aug 10, 2017

WillieCubed deleted the patch-1 branch August 11, 2017 05:23

Update README.md to include app_name and color requirements #862

Update README.md to include app_name and color requirements #862

Uh oh!

Conversation

WillieCubed commented Aug 9, 2017

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillieCubed Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillieCubed Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samtstern commented Aug 9, 2017

Uh oh!

WillieCubed commented Aug 9, 2017

Uh oh!

samtstern commented Aug 9, 2017

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WillieCubed Aug 9, 2017 •

edited

Loading

WillieCubed Aug 9, 2017 •

edited

Loading