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

Android: handle failed attempts with error message, icon color and description. #163

Merged
merged 2 commits into from
Oct 27, 2018
Merged

Conversation

srulfi
Copy link
Contributor

@srulfi srulfi commented Oct 12, 2018

  • Current behaviour: Android, unlike iOS, closes the dialog after the first attempt.
  • Expected behaviour: Handle errors and allow system 5 attempts (default) before disabling sensor. Inform the user about the error.

Issue discussed 156, 81, 92, 107

Updated UI:
screen shot 2018-10-12 at 11 52 21

Updated config object:

  • replaced color for imageColor
  • added imageErrorColor (default: "#ff0000")
  • added sensorErrorDescription (default: "Failed")

@zibs
Copy link
Collaborator

zibs commented Oct 23, 2018

This is really interesting - thanks for the PR. I'll review and think about how best to integrate since this changes the flow for existing users.

@ronilitman
Copy link
Contributor

This is great. Hope this comes in soon

@srulfi
Copy link
Contributor Author

srulfi commented Oct 25, 2018

@zibs I understand, thanks for taking the time to review it. @ronilitman glad you find it useful, hopefully it'll get merged.

@zibs
Copy link
Collaborator

zibs commented Oct 27, 2018

What do you all think of #165 ? It seems like it might be a good time to migrate to what will become the conventional UI for android?

@zibs
Copy link
Collaborator

zibs commented Oct 27, 2018

Nice, this is pretty awesome. I'll merge it in.

@zibs zibs merged commit ad0eeb8 into naoufal:master Oct 27, 2018
@ronilitman
Copy link
Contributor

ronilitman commented Oct 28, 2018

@srulfi Thanks for those changes, this is very helpful!
2 things that I found out though:

  1. Now that color changed to imageColor, the text color on the bottom right can't be changed.
  2. Maybe we should consider to have the ability to change the text after user had too many attempts of the bottom text to something a user can decide, such as "show password form" with a callback. What do you think?

@ronilitman
Copy link
Contributor

also, having this issue, not sure this is related to this PR:
#171

@srulfi
Copy link
Contributor Author

srulfi commented Oct 29, 2018

@zibs in regards of #165 I agree it would be a good time to migrate from the deprecated FingerprintManager to BiometricPrompt. I've done my share of research and tests a while back and actually came across the example you listed in the PR so I will definitely help to make it happen.

@srulfi
Copy link
Contributor Author

srulfi commented Oct 29, 2018

@ronilitman the issue you mentioned seems to be reproducible on iOS and the significant files this PR modified where all on Android side. For what it's worth I'm not able to reproduce the issue you described. Tested it only on the simulator. Let me know if I can help you troubleshooting it.

Regarding the 2 things you listed:

  1. The adding of the imageColor prop required a semantical differentiation from the previously used color which tied both sensor image and cancel button under dialogColor (native side). Personally I don't think is very useful to change the color of the cancel button and it seemed a bit arbitrary to leave out other elements like the title, reason and description. I think it's valid to allow the configuration of each element on the dialog but should be done semantically, so if you find it useful we can surely add a cancelColor prop to the config object.
  2. The passcode fallback was a feature long discussed across PRs and issues. Originally it exceeded the scope of the library but there was some recent activity on that front and it appears to have been implemented for iOS #101. I can assume then that it should also be reflected on Android but it will probably have to wait for the #165 @zibs mentioned above to avoid development time loss.

@zibs
Copy link
Collaborator

zibs commented Oct 30, 2018

@srulfi Awesome to hear about #165 - I'll be able to take a deeper look into this weekend.

@JeffreyLeeDave
Copy link

My issue with this change currently, and I don't know how it was before, is that even though it disables the sensor after 5 attempts, there is no trigger for us to do anything on failure of those 5 attempts. Or am I missing something

@ghost
Copy link

ghost commented Sep 9, 2019

Is there is any possibility to increase attempts to more than 5? Or can be any callback that invokes after 5 attempts?

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