Skip to content

Conversation

@mjmarathon
Copy link

@mjmarathon mjmarathon commented Nov 23, 2017

This pull request encompasses two issues.

  1. With the release of iOS 11.0 Apple released FaceID. Fortunately all of the calls are identical to using TouchId so there weren't any changes needed there. However, the current property names and text on the view controller do not reflect that. They were changed to a more generic "Biometric Authentication" to better reflect functionality. A potential improvement for this is that as of iOS 11.0 the device can differentiate between which functionality the device supports and we could change the text of the button on the controller to reflect this.

  2. Currently the only cancellable lock type is .remove which is done via the declaration of the type. This really doesn't make any sense. We should allow users to decide which locks are and are not cancellable. I have added a property to the PasscodeLockConfigurationType class which the view controller will key off of to show/hide the cancel button. Personally I think this makes more sense as the cancellation seems more like a configuration thing rather than a lock type thing.

…o accommodate FaceID. Also moved "allowsCancellation" to PasscodeLockConfiguration to allow better flexibility for users.
@ziogaschr
Copy link
Collaborator

@mjmarathon nice work man, I will try to review it locally soon. I don't see any changes to the strings file and also the image for TouchID (fingerprint) in the UI remains the same, do you think it will be a good idea to change those too? Can we somehow check if the supported method is TouchID or FaceID?

@mjmarathon
Copy link
Author

@ziogaschr In the order of things you asked.

  1. Looking at the strings file I can see that we're using "PasscodeLockTouchIDReason" as a default when we're presenting the TouchID prompt. FaceID will use this in the same way so no change is needed other than to make the naming scheme more accurate.
  2. I am not seeing which images you're referring to, if you mean on the prompt for face/touchid itself those are handled by the OS. It that's not what you're talking about lemme know where these images are referenced and i'll take a look.
  3. FaceID is only supported by iOS 11 and up. With that OS release we can indeed check which method is supported. The way apple handled this code makes it seem like a device is either going to have face OR touch and not both. Personally I've found handling this to be a bit of a pain in the ass to deal with all the @avaliables needed for UI to reflect which is supported by the device but I can take another look at it if you'd like.

@ziogaschr
Copy link
Collaborator

Hey @mjmarathon, sorry for the long wait. I am checking the code now.

Can you explain why have you replaced isCancellableAction from PasscodeLockStateType with PasscodeLockConfigurationType .allowsCancellation. I don't think this is bad, but for this to work for all use cases we will have to make the PasscodeLockConfigurationType copyable by implementing the NSCopying, this way it might be easier for someone to change this or any value for a specific PIN display.

Although despite what we think, a lot of people are using this project already and we have to keep it backwards compatible, so this change will break all these apps. Such change, if needed, might be fine to be added in a next major release (v2.0).

For the rest methods that you've renamed TouchID to Biometrics, I liked them, but we can keep the old ones pointing to new ones, at least for v1.0 with a deprecation warning.

What do you think? Are you fine if I make changes to your PR or will you like to make them your own?

@ziogaschr
Copy link
Collaborator

@mjmarathon Please check #54 for FaceID (based on your PR 👍 ). For the cancelable actions, please make a new PR explaining the reasons for it. :)

@ziogaschr ziogaschr closed this Jan 10, 2018
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