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

Default DismissButtonStyle when using SFSafariViewController iOS 11+ #305

Merged
merged 14 commits into from
Sep 16, 2019

Conversation

andrewfoghel
Copy link
Contributor

@andrewfoghel andrewfoghel commented Sep 1, 2019

Issue: #283
Issue: https://community.auth0.com/t/done-button-in-webview/23718/4

Description

This PR handles setting the SFSafariViewController.dismissButtonStyle to .cancel when using the useLegacyAuthentication(withStyle:) method. Above are 2 places where people have asked for this feature, and I'm sure they aren't the only ones who need this support.

Testing

We've added a spec to ensure that this gets set properly when using useLegacyAuthentication(withStyle:), however; the test will only run on iOS 11.0 and up. If there are any ideas for testing this please let me know.

Screenshots/GIFs

Screen Shot 2019-09-01 at 12 51 14 AM

@andrewfoghel andrewfoghel requested a review from a team September 1, 2019 05:13
@andrewfoghel
Copy link
Contributor Author

Closing as there are failures, going to investigate.

@andrewfoghel andrewfoghel reopened this Sep 1, 2019
@cocojoe
Copy link
Member

cocojoe commented Sep 4, 2019

@andrewfoghel Thanks for the PR and screenshot. What is the use case? I noticed the other issues but they also give no reason as to why it's important.

Looking at the above I feel that the current default Done is not really the optimal value and the value should instead align with Apple's recommended WebAuth flow in iOS 11+. This uses Cancel which makes sense from an end user perspective as you are cancelling Web Auth.

So I think it should default to Cancel to match Apple's behaviour. However, not sure on it being configurable. Does this align with why you wanted to change it?

@andrewfoghel
Copy link
Contributor Author

@cocojoe Thanks for the response here! This use case is more so for people using the useLegacyAuthentication() helper. It gives people access to customize that button when using an SFSafariViewController. I believe that when using ASWebAuthenticationSession it is always Cancel and that you can't change that value. However, for SFSafariViewController it gets defaulted to Done so allowing them to change it to Close or Cancel gives them the opportunity to maintain more consistency throughout their application. Thoughts?

@andrewfoghel
Copy link
Contributor Author

We will not be using the useLegacyAuthentication() helper anymore, so we personally don't need this as a part of the SDK. However, I don't want to take from the community that uses that method, thus; I'll leave this open. Feel free to close this if this functionality is deemed as unnecessary or overkill. Thanks for the careful consideration with this!

On a separate note, if you all decide that you want this feature in I will maintain this PR.

@cocojoe
Copy link
Member

cocojoe commented Sep 5, 2019

@andrewfoghel I feel we can address the root cause here changing the default of the Library to specify Cancel which aligns with Apple's current default messaging in ASWebAuthentictionSession.

This should address the concern without having to expose additional methods to allow this to be changed by the developer.

@lbalmaceda lbalmaceda requested review from cocojoe and removed request for a team September 5, 2019 22:18
@andrewfoghel
Copy link
Contributor Author

@cocojoe I was thinking it would be nice to give the developer the option to choose whichever option they want; however, I'm not opposed to simply defaulting the button to say Cancel to match ASWebAuthenticationSession. Those changes should be reflected in the last few commits, let me know what you think!

@cocojoe
Copy link
Member

cocojoe commented Sep 16, 2019

@andrewfoghel Thanks for changing, I was on vacation, hence the delay in response :) Looks good 👍

@cocojoe cocojoe merged commit 52c6b15 into auth0:master Sep 16, 2019
@cocojoe cocojoe changed the title Add Ability To Set DismissButtonStyle When Using SFSafariViewController Default DismissButtonStyle when using SFSafariViewController iOS 11+ Sep 16, 2019
@cocojoe cocojoe added this to the vNext milestone Sep 16, 2019
@andrewfoghel
Copy link
Contributor Author

@cocojoe thanks for the approval here! I'm going to delete the branch for prosperity, til next time! (:

@andrewfoghel andrewfoghel deleted the handle-setting-safari-dismiss-style branch September 18, 2019 04:55
@cocojoe cocojoe mentioned this pull request Sep 20, 2019
sam-w pushed a commit to LoungeBuddy/Auth0.swift that referenced this pull request Mar 11, 2020
…uth0#305)


* Default DismissButtonStyle to cancel to match newer methods.
sam-w added a commit to LoungeBuddy/Auth0.swift that referenced this pull request Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants