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

make isSupported return an error when TouchID and FaceID is not supported #177

Closed
wants to merge 3 commits into from

Conversation

philolo1
Copy link

@philolo1 philolo1 commented Nov 9, 2018

Due to recent changes "TouchID" is returned when "Passcode" is supported. I deleted this check, as most of the users don't expect to only return "TouchID" and "FaceID" not passcode.

@zibs
Copy link
Collaborator

zibs commented Nov 9, 2018

Based from #101 we want to allow passcode functionality, so we need to be able to communicate that passcode may be available, even if touchid/faceid is not.

@philolo1
Copy link
Author

@zibs we can also return "Passcode" instead, but i think its quite confusing for an initial user of this framework.

@philolo1
Copy link
Author

@zibs this is the other solution, where you would return "Passcode". However i dont really like this, as the normal behaviour should be that its not supported unless you want it to include a passcode

@zibs
Copy link
Collaborator

zibs commented Nov 10, 2018

I think it's for the best tbh. I think in bringing in passcode might have muddied the implementation a bit, but at least now calling isSupported will return what is supported on iOS (face/touch/passcode), letting the user make the choice of how to authenticat (using the passcodeFallback, or not, for example.) I'll think about it a bit more, and then test and merge if it's all good.

@zibs
Copy link
Collaborator

zibs commented Nov 11, 2018

@philolo1 haha you know what - I think you're right after thinking about it more. We should return false/error if isSupported does not support touch-id/face id.

Can you revert your commit back to your original commit?

@philolo1
Copy link
Author

@zibs sorry to keep you waiting. Its done now.

@pilsy
Copy link
Contributor

pilsy commented Nov 27, 2018

This provides a much nicer solution ~ #182

@zibs
Copy link
Collaborator

zibs commented Feb 15, 2019

I merged #182 and don't think we need this anymore!

@zibs zibs closed this Feb 15, 2019
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.

4 participants