Skip to content
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

[Do Not Merge] Sign In With Apple #953

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

arnavs-0
Copy link

Added Initial Sign In with Apple.

Summary:
Added Initial Sign In with Apple. We need to return the user for the profile. Clean up Code, Add Apple Developers to Firebase. We need to add a warning for using user data.

Test Plan:
Still under development. Need to add all features.
Screenshots:
redirect
Home
redirect load

Added Initial Sign In with Apple. Need to get return user info. Add User Accept Permissions and Apple Dev Account
@bherbst
Copy link
Contributor

bherbst commented Jun 23, 2020

How many Android users do we expect regularly use Apple accounts?

Copy link
Contributor

@bherbst bherbst left a comment

Choose a reason for hiding this comment

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

Looks like you might have accidentally included some hprof files as well- I'm guessing we don't need those for anything.

@@ -94,7 +94,7 @@ android {

defaultConfig {
applicationId "com.thebluealliance.androidclient"
minSdkVersion 16
minSdkVersion 17
Copy link
Contributor

Choose a reason for hiding this comment

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

@phil-lopreiato any stats on users on API level 16?

Honestly I'd be in favor of going to 21 or 23 soon, but just curious on whether there's any drive to keep it lowerl

Copy link
Author

Choose a reason for hiding this comment

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

If it is a must we can keep Android 16. I just used a dependency for the Sign In with Apple button so I could test really quickly. If we needed I can make a button just thought this would be easier lol. The dependency needed a minimum of API 17

Copy link
Member

Choose a reason for hiding this comment

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

I've always just kept it where it was until we had a compelling enough reason to bump it. Bumping to 17 should have practically no effect

@@ -215,6 +215,7 @@ repositories {
maven {
url "http://github.com/wada811/Android-Material-Design-Colors/raw/master/repository/"
}
mavenCentral()
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you having issues resolving the new dependency without this? The top level build.gradle already defines mavenCentral() as a repo: https://github.com/the-blue-alliance/the-blue-alliance-android/blob/master/build.gradle#L41

Copy link
Author

Choose a reason for hiding this comment

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

I did encounter dependency issues so I tried adding a few things and it worked when I added it worked. I'll try again to see if it was just Android Studio acting up.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching this. It worked, I guess Android Studio was acting up when I ran it.

# Specifies the JVM arguments used for the daemon process.
# The setting is particularly useful for tweaking memory settings.
# Default value: -Xmx10248m -XX:MaxPermSize=256m
# Default value: -Xmx1024m -XX:MaxPermSize=256m
Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, that was definitely wrong before.

@arnavs-0
Copy link
Author

How many Android users do we expect regularly use Apple accounts?

I don't exactly know how much we can expect. However, I came across a couple of articles explaining why we should have it as TBA has an iOS app. It gives people who have an android device to sign in with their own apple id. More and more people are getting apple ids from Android. I am not sure how it will impact us, however, I am seeing more Sign in With Apple on Android Apps.

@ZachOrr
Copy link
Member

ZachOrr commented Jun 23, 2020

@bherbst (it doesn’t seem like I can reply inline) re: how many Android users will use Sign In with Apple - the motivation behind this is Apple is requiring us on iOS to support Sign In with Apple, since we offer other social logins. Currently, we can’t push updates to the App Store (we’ve been blocked since April-ish) because we’re not in compliance with this new rule yet. Part of the challenge is we need to enable users to use Sign In with Apple on all of our platforms. So although signing in with an Apple ID will be rare on Android, we need it to ensure if users start their account on web with Sign In with Apple, they can pickup on Android

@bherbst
Copy link
Contributor

bherbst commented Jun 23, 2020

Ugh, forgot about that requirement for Apple sign-in. Big old 👎 to Apple.

arnavs-0 and others added 2 commits June 23, 2020 11:37
@arnavs-0 arnavs-0 changed the title [Do Not Merge] Added Initial Apple Sign In [Do Not Merge] Sign In With Apple Dec 7, 2020
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.

None yet

4 participants