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

#166 Implement apple web based authentication using react native WebView. #177

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Romick2005
Copy link

Can you please review it and let me know if this will work for this module.

@CLAassistant
Copy link

CLAassistant commented Dec 16, 2020

CLA assistant check
All committers have signed the CLA.

README.md Outdated
console.log("onAppleAuthResponse responseContent", responseContent);
}

// Apple authentication requires API 19+, so we check before showing the login button
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see any API 19+ check?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see the "suggested" block below, might be best to remove this comment, and on top of the "suggested" (commented-out) code block before, mention "It makes sense to show the native buttons in a real app, something like the code below, but we are just demonstrating web login here so it is commented out"

@mikehardy
Copy link
Collaborator

@Romick2005 thanks for that change - this looks pretty cool as I read it. Are you using this in your app / can you attest that this works for you exactly as is ?

@Romick2005
Copy link
Author

@Romick2005 thanks for that change - this looks pretty cool as I read it. Are you using this in your app / can you attest that this works for you exactly as is ?

Yes it is working for me. Should I create some tests or examples?

@mikehardy
Copy link
Collaborator

Implementing it in the example app itself is the gold standard - I suppose that may not be possible without my help in order to get the SHA and everything configured correctly? If you implemented it there so that it was working for everything except those bits I can work that out so that it works out of the box I think, then it serves as the test too (it's what I use to test larger PRs, anyway)

@Romick2005
Copy link
Author

@mikehardy I add examples, but I have no chance to test them. Would be good if you could test it on your side. Please let me know if I need to complete anything for this pull request.

@mikehardy
Copy link
Collaborator

@Romick2005 thank you for your patience! I was travelling which cause a delay in this. It appears the CLA is still not signed, so I won't be able to integrate this yet. Could you follow the CLA instructions and sign it please? Thank you!

@mikehardy mikehardy added cla documentation Improvements or additions to documentation labels Dec 18, 2020
@Romick2005
Copy link
Author

@mikehardy Done! Thank you!

@Romick2005
Copy link
Author

@mikehardy Is there anything I can help you with?

@mikehardy
Copy link
Collaborator

Not that I'm aware of - it's just holiday time and I have a small child so I'm not working very much

@mikehardy mikehardy self-assigned this Feb 12, 2021
@vtoupet
Copy link

vtoupet commented Mar 9, 2021

Looking forward to this PR integration. Having Apple Sign-In on iOS < 13 would be a great addition. End-users are surprised to have it on Android even before all iOS versions.

@vtoupet
Copy link

vtoupet commented Mar 14, 2021

@mikehardy any idea when you plan to review this PR?

@mikehardy
Copy link
Collaborator

Very sorry :-), one of my projects (https://github.com/ankidroid/Anki-Android/) was selected to participate in Google Summer of Code and we had literally 50 people show up and start posting PRs all at once. I got 500 notifications in a week 😅 - it has taken almost all of my volunteer time.

I really appreciate your patience - it will happen, hopefully soon, I'm nearly caught up again despite the torrent of PRs in the other project

@suomimammutti
Copy link

@mikehardy Any chance of getting this reviewed & merged ?

@paulschreiber
Copy link

We would really like to see this, too. Anything we can do to help @mikehardy?

@Romick2005 can you update the fork/branch to resolve the merge conflicts?

mikehardy pushed a commit that referenced this pull request Nov 8, 2023
@mikehardy
Copy link
Collaborator

I rebased it and pushed the result to #335 - it needs testing, and I believe there are comments here that were not addressed

The way to move this forward is to test it, as mentioned on #335, and to look at any of my old comments here and resolve them

Would be cool to see it go in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants