search

smstuebe / xamarin-fingerprint

Xamarin and MvvMCross plugin for authenticate a user via fingerprint sensor
Microsoft Public License
491 stars 116 forks source link

A few small recommendations #8

Closed jamesmontemagno closed 8 years ago

jamesmontemagno commented 8 years ago

First, this is awesome!

Just reading through at a high level the implementation and code could I recommend some of the following:

  1. Current Activity Resolver: I think this may be a bit problematic as the Activity can change from time to time. Would it just be easier to have it use CrossCurrentActivity and have no additional setup?
  2. Is it necessary to actually have the Mvx Plugin? If you do ^ then there would be no need and no need to manage multiple nuget packages since all Plugins for Xamarin are Mvx Compatible.
  3. Add Permission Automatically: See how I do it here for Android: https://github.com/jamesmontemagno/Xamarin.Plugins/blob/master/Contacts/Contacts/Contacts.Plugin.Android/Properties/AssemblyInfo.cs#L32 and this would get ride of your checks in "CheckAvailability" in your android project
  4. Perhaps breaking IsAvailable into multiple bool properties for "HasSensor", "IsEnrolled" and IsAvailable would be all of these combined.
  5. Adjust logo based on branding guidelines: https://xamarin.com/branding I would just use the fingerprint logo without the xamagon.
  6. Add blank implementations for anything not supported (this way it doesn't throw an exception) and would just return false or is not available. You can see what I did with my permissions plugin.

The other things that would be nice if just naming convention to match what I am trying to have everyone standardize on to make it really easy for devs to get running:

  1. Namespace: Plugin.Fingerprint
  2. Singleton Class: I really like "Cross"+FeatureName, but that is me, so "CrossFingerprint" not sure why I went with this originally, but it kind of stuck.
smstuebe commented 8 years ago

Thanks for the feedback. :+1:

  1. No. This is a callback which should return the current activity. The callback is called right before creating the dialog. I want to avoid adding dependencies to other plugins. That's why I added it to the description.
  2. The Mvx Plugin uses Mvx mechanisms like IMvxAndroidCurrentTopActivity, register plugin implementation for dependency injection. I don't want to lose it. Building both plugins is just one command for me :stuck_out_tongue_closed_eyes: Aaaaaaand: I'm a MvvMCross fanboy. :sparkles:
  3. I'll have a look.
  4. Ok, I'll implement it
  5. Adjusted, sorry for that. On nuget It will be changed with the next nuget release.
  6. You mean for Windows Phone, etc.?

Finding namespaces for hobby projects is always a pain. I'll leave it like this for now and think about it :smile_cat: I have to clean up some stuff, anyway.

smstuebe commented 8 years ago

Fixed namespaces :)