-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split GitHub provider out into its own module #1421
Split GitHub provider out into its own module #1421
Conversation
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
auth/src/main/res/values/config.xml
Outdated
<string name="facebook_login_protocol_scheme" translatable="false">fbYOUR_APP_ID</string> | ||
|
||
<!-- |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
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).
🤣 TODO(me):
|
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
<category android:name="android.intent.category.BROWSABLE" /> | ||
|
||
<data | ||
android:host="@string/firebase_web_host" |
There was a problem hiding this comment.
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:
- I see we have duplicate
strings.xml
entries forfirebase_web_host
in bothauth/.../strings.xml
andauth-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. - 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 toCHANGE-ME
? I think that might be helpful.
There was a problem hiding this comment.
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 😁:
FirebaseUI-Android/auth/src/main/java/com/firebase/ui/auth/AuthUI.java
Lines 973 to 977 in f83c621
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, |
- We need
firebase_web_host
in the auth module to do that check - We do that check 😂
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
FirebaseUI-Android/auth/build.gradle.kts
Line 25 in df554e2
implementation(Config.Libs.Support.customTabs) |
Fixes #1389 and #1412