Some class from sample app should be in the library

airbnb / mavericks

Mavericks: Android on Autopilot

https://airbnb.io/mavericks/

Apache License 2.0

5.83k stars 500 forks source link

Some class from sample app should be in the library #82

Closed eboudrant closed 6 years ago

eboudrant commented 6 years ago

While testing MvRx it happened I did a lot of code copy/paste from the sample app. Mainly theses classes : MvRxEpoxyController, BaseFragment (minus the androidx.navigation part), MvRxViewModel. I think it would really help to have theses class in the library. Especially the epoxy related classes. May be in an optional library when we combine MvRx with Epoxy. To add on this, some of the class from sample app are referenced in the doc, it is a bit confusing to not see them in the main library (ex: https://github.com/airbnb/MvRx/wiki/Advanced-Concepts#mvrxviewmodel). Thanks!

elihart commented 6 years ago

I think it's reasonable to provide some of this in a module that we ship.

The downside is that so much of the fragment implementation is up to how you choose to use MvRx, so it's hard to provide a one size fits all solution.

Playing devil's advocate, how hard is it to copy 1 or 2 files? We could update the documentation to be more clear about where the files live.

gpeal commented 6 years ago

@eboudrant @elihart There was a reason for each one:

MvRxViewModel: it requires you to pass debugMode into super. You should use the BuildConfig constant from your application package which we can't do for you.
BaseMvRxFragment. You should also have a BaseFragment in your app in which you can put shared code. We recommend having your app's BaseFragment extend BaseMvRxFragment and do not recommend using BaseMvRxFragment directly.

I get that this is slightly confusing so I added a few sentences to this wiki page explaining this.