search

MvvmCross / MvvmCross-AndroidSupport

Android support library packages for MvvmCross: The .NET MVVM framework for cross-platform solutions.
http://mvvmcross.com
15 stars 0 forks source link

AppCompatDialogFragment (v7) vs DialogFragment (v4) #209

Closed IlSocio closed 8 years ago

IlSocio commented 8 years ago

I think that both the:

should be moved to the MvvmCross.Droid.Support.V4 namespace, because these classes are based on DialogFragment (v4)

And two different classes should be added under the MvvmCross.Droid.Support.V7.AppCompat namespace... (or maybe under MvvmCross.Droid.Support.V7.Fragging?)

which will be based on AppCompatDialogFragment (v7) http://developer.android.com/intl/zh-cn/reference/android/support/v7/app/AppCompatDialogFragment.html

Is it right?

martijn00 commented 8 years ago

It seems you are right! Could you make a PR to fix this? The MvxAppCompatDialogFragment should go under the AppCompat library.

IlSocio commented 8 years ago

Actually, is the MvvmCross.Droid.Support.V7.Fragging namespace really needed? It seems to be inconsistent against the naming convention of the official support library...

IlSocio commented 8 years ago

Sure Martin, I'll try to create a PR ;)

martijn00 commented 8 years ago

It might not be needed. I think we introduced it to separate the whole fragment thing a while ago. Maybe we should refactor this and put it into the official structure? What does @Cheesebaron think?

Cheesebaron commented 8 years ago

So yes.

IlSocio commented 8 years ago

Since the MvxAppCompatDialogFragment will be strictly dependent from the AppCompat, wouldn't it better to place it under the v7.AppCompat namespace?

By placing it under the v7.Fragging we'll introduce a new nuget dependency and people who installs the v7.Fragging package will be forced to install the v7.AppCompat package too.

IlSocio commented 8 years ago

I've just submitted a PR for moving all the V4 classes to the proper namespace. I'll submit a different PR for the MvxAppCompatDialogFragment.

martijn00 commented 8 years ago

I've merged this through #214 Please check if everything works as you would expect.

IlSocio commented 8 years ago

I checked and it's perfect Martijn! ;)