Skip to content

Conversation

@SUPERCILEX
Copy link
Collaborator

Fixes #1389 and #1412

@SUPERCILEX SUPERCILEX requested a review from samtstern as a code owner August 27, 2018 05:39
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM! Just some very small comments.

Also do you think this means we need to go to 5.0.0?

I kinda wanted to save v5 for something cool but I guess I am being overly sentimental about versioning.

@@ -1,4 +1,4 @@
package com.firebase.ui.auth.data.remote;
package com.firebase.ui.auth.github;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in .github.data.remote just to be consistent?

-->
<string name="facebook_login_protocol_scheme" translatable="false">fbYOUR_APP_ID</string>

<!--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to kill these comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our documentation in the config.xml is different in the auth vs app module, so I was trying to unify them. I've made it so auth is the documented one with app pointing to it. WDYT? Should it be flipped?

@samtstern samtstern added this to the 4.2.0 milestone Aug 27, 2018
@SUPERCILEX
Copy link
Collaborator Author

SUPERCILEX commented Aug 28, 2018

Regarding 5.0, I don't think so? Everything will still compile as before, it'll "just" be a runtime crash. I don't think we need to bump to 5.0 because people have to update their deps anyway to get this change (so it's not super hard to add another dep).

I am being overly sentimental about versioning

🤣

TODO(me):

  • Check release builds
  • Update documentation

Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX SUPERCILEX changed the title [WIP] Split GitHub provider out into its own module Split GitHub provider out into its own module Aug 28, 2018
<category android:name="android.intent.category.BROWSABLE" />

<data
android:host="@string/firebase_web_host"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok two questions about this string:

  1. I see we have duplicate strings.xml entries for firebase_web_host in both auth/.../strings.xml and auth-github/.../strings.xml ... is there a reason it needs to exist outside of the GitHub module? I feel like we only use it there but could be forgetting something.
  2. Now that we're sure you want to use GitHub auth when you include the -github module, should we explode at runtime if this string is still equal to CHANGE-ME? I think that might be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, both those birds were killed with the same stone in the original implementation 😁:

Preconditions.checkConfigured(getApplicationContext(),
"GitHub provider unconfigured. Make sure to add your client id and secret." +
" See the docs for more info:" +
" https://github.com/firebase/FirebaseUI-Android/blob/master/auth/README.md#github",
R.string.firebase_web_host,

  1. We need firebase_web_host in the auth module to do that check
  2. We do that check 😂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh

// Needed to override Facebook
implementation(Config.Libs.Support.cardView)
implementation(Config.Libs.Support.customTabs)
implementation(Config.Libs.Support.cardView) // Needed to override Facebook
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something change about how Facebook uses custom tabs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, but we use custom tabs ourselves (for the ToS and PP) so it was a duplicate dependency:

implementation(Config.Libs.Support.customTabs)

@samtstern samtstern merged commit 68be249 into firebase:version-4.2.0-dev Aug 29, 2018
@SUPERCILEX SUPERCILEX deleted the github-split branch August 29, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants