Skip to content

Add Twitter as an IDP #268

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

Merged
merged 4 commits into from
Aug 26, 2016
Merged

Conversation

amandle
Copy link
Contributor

@amandle amandle commented Aug 26, 2016

Add Twitter as a provider in response to issue #157

I am not entirely sure how this will fit in with our feature parity plans with other platforms, we may have to wait for iOS to implement it before we can actually release.

Also, the Twitter Client Secret is stored in a resources file, which is not ideal but without writing a custom back-end server to coordinate with I'm not sure there is a better alternative (just obfuscation techniques) though I'm open to any suggestions.

@@ -80,5 +80,9 @@
</intent-filter>
</activity>

<meta-data
android:name="io.fabric.ApiKey"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually necessary? In the Firebase Auth quickstart I was able to get Twitter Sign In to work with only the consumer key/secret and didn't need the app ID or any meta-data:

https://github.com/firebase/quickstart-android/blob/master/auth/app/src/main/AndroidManifest.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it doesn't appear to be necessary. Removed it.

@samtstern
Copy link
Contributor

@amandle this looks really good! We do need to wait for iOS to release but I think we can still do work like this on a vNext branch or similar. Can we move this PR to targeting a version-1.0.0-dev branch?

@amandle amandle changed the base branch from master to version-1.0.0-dev August 26, 2016 21:19
@@ -229,6 +245,17 @@ private boolean isFacebookConfigured() {
}

@MainThread
private boolean isTwitterConfigured() {
List<String> twitterConfigs = Arrays.asList(
getResources().getString(R.string.twitter_app_id),
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed one reference to twitter_app_id

@samtstern
Copy link
Contributor

LGTM, thanks!

@samtstern samtstern merged commit e3a731a into firebase:version-1.0.0-dev Aug 26, 2016
samtstern pushed a commit to samtstern/FirebaseUI-Android that referenced this pull request Sep 13, 2016
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