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

Add tests for event page #56

Open
michaelbukachi opened this issue Oct 4, 2019 · 15 comments
Open

Add tests for event page #56

michaelbukachi opened this issue Oct 4, 2019 · 15 comments

Comments

@michaelbukachi
Copy link
Collaborator

This is a sub issue for #53

@johnGachihi
Copy link
Contributor

I can work on this

@wangerekaharun
Copy link
Collaborator

Feel free to work on it

@johnGachihi
Copy link
Contributor

johnGachihi commented Dec 1, 2019

@wangerekaharun is it ok if I move all the Firebase Remote Config code from EventsFragment into a new repository. This will make it a lot easier to test the fragment

@wangerekaharun
Copy link
Collaborator

wangerekaharun commented Dec 1, 2019

Yes its okay to do so. Do the same for all Remote Config code.. We were supposed to do so but you can take the task.

@johnGachihi
Copy link
Contributor

Alright

@johnGachihi
Copy link
Contributor

johnGachihi commented Dec 1, 2019

Also @wangerekaharun can I ask what the reason is for having the getter methods for the livedata in the viewmodels. Are they really necessary?

@wangerekaharun
Copy link
Collaborator

More of separation of concerns. Having one method doing the fetch and the other one doing the get response from the livedata. This also helps in orientation changes. Assuming you only have one method for fetching and updating the livedata, it will be called every time we have orientation change meaning livedata will be updated again. In the current approach, since the fetch methods are called when needed, the livedata will still have their values even on orientation change

@johnGachihi
Copy link
Contributor

If instead of using the livedata getters, we accessed the livedata directly (like this: myViewModel.myLiveData) would it make a difference.

@wangerekaharun
Copy link
Collaborator

Why would you want it to be so?

@johnGachihi
Copy link
Contributor

I feel like the getters are not necessary. The livedata properties could just be made public and accessed directly.

@wangerekaharun
Copy link
Collaborator

@michaelbukachi what's your take on this?

@michaelbukachi
Copy link
Collaborator Author

I usually make live data public. It's just easier since it reduces the amount of code written.

@wangerekaharun
Copy link
Collaborator

@johnGachihi you can go ahead and make the changes

johnGachihi added a commit to johnGachihi/droidconKE2019App that referenced this issue Dec 9, 2019
@johnGachihi
Copy link
Contributor

johnGachihi commented Dec 9, 2019

@johnGachihi you can go ahead and make the changes

@wangerekaharun I've found that it's good practice to use MutableLiveData in viewmodels but expose the immutable LiveData to Activities and Fragments like you had done.
But I feel like this example is more Kotlin-like than using getter methods

@wangerekaharun
Copy link
Collaborator

Yes we already had agreed on this as @michaelbukachi had pointed out

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

No branches or pull requests

3 participants