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

Made the value of reason in authenticateWithBiometrics injectable #21

Merged
merged 1 commit into from
Jun 12, 2016

Conversation

phampton24
Copy link

I've coded a potential solution to issue #20. This solution leverages PasscodeLockConfigurationType to optionally pass a value leveraged by authenticateWithBiometrics. If the value is not set in the implementation of the Configuration Type then the code falls back to the existing logic that uses the strings resource file.

@ziogaschr
Copy link
Collaborator

Looks ok, and I agree that more people might need this. I didn't have the time to test it though.
@velikanov what do you think? I will try to test it during the weekend

@velikanov
Copy link
Owner

@ziogaschr, unfortunately, I just don't have enough time even to test it :( too busy with my main job
feel free to make anything that will bring us to success :)
cheers!

trispo added a commit to kfinteractive/SwiftPasscodeLock that referenced this pull request May 26, 2016
@ziogaschr
Copy link
Collaborator

I have tested this and it is great.
Although it will break current Apps that are using this framework, so I vote for keeping it for the next major release v2.0.0.

What do you think?

@phampton24
Copy link
Author

@ziogaschr I'm curious about your statement that the changes will break current Apps that are using the framework. My intention was to make the changes fully backwards compatible for existing user's of the framework, which I thought I did by making the touchIdReason optional on PasscodeLockConfigurationType. If there's something I overlooked, though, I'd appreciate your feedback so that I can file it away for future reference.

@ziogaschr
Copy link
Collaborator

You are tottaly right @phampton24. I thought that var touchIdReason: String? = nil will have to be implemented in the Apps code, otherwise it will threw an error. I just tested it and it works ok.

@ziogaschr ziogaschr merged commit d9503fc into velikanov:master Jun 12, 2016
@ziogaschr
Copy link
Collaborator

Grrrr, I had the same issue #30 at the beginning.
Although, it wasn't a problem last time I tested it, before doing the merge.

@ziogaschr
Copy link
Collaborator

Shall we revert this one and has already caused trouble to people

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