Skip to content

barebones to get live data and data binding working#1

Open
ahaly92 wants to merge 7 commits intomasterfrom
ahmed/live-data
Open

barebones to get live data and data binding working#1
ahaly92 wants to merge 7 commits intomasterfrom
ahmed/live-data

Conversation

@ahaly92
Copy link
Owner

@ahaly92 ahaly92 commented Apr 20, 2019

No description provided.

README.md Outdated

Architecture: MVVM
Language: Kotlin
Architectural Components: LiveData, Data Binding
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android* Architecture*

import android.view.ViewGroup
import android.widget.EditText
import com.example.livedata.databinding.FragmentLoginBinding

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

Copy link

@abesary abesary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥
a few comments but looks good

override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View? {
super.onCreate(savedInstanceState)
loginViewModel = ViewModelProviders.of(this).get(LoginViewModel::class.java)
binding = DataBindingUtil.inflate(inflater, R.layout.fragment_login, container, false)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use the autogenerated binding class to inflate if you want it to be more specific. FragmentLoginBinding.inflate(inflater)

it.lifecycleOwner = this
it.loginViewModel = loginViewModel
return it.root
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since fragment lifecycles are a bit more complicated than activity's lifecycle (and causes issues, you can read about it here), they added a lifecycle that represents the fragment's. Use getViewLifecycleOwner() instead.

I think it'd be cleaner if you separate the field initialization and the return lines like:

return binding.apply {
    lifecycleOwner = getViewLifecycleOwner()
    loginViewModel = loginViewModel
}.root

But not necessary

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work without having to name the private variable loginViewModel something different. apply changes scope to this so any reference to loginViewModel will imply this. loginViewModel = this.loginViewModel

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you use let it.loginViewModel is the attribute on binding and loginViewModel is the private variable in the Fragment

binding.run {
user?.let {
when {
it.emailAddress.isEmpty() -> setErrorOnField(emailAddress, R.string.empty_email_error)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't all this logic be in view model? You can expose an error live data field that emits what kind of error to show and the fragment only knows about displaying error with the info vm emits.

import android.view.View

class LoginViewModel : ViewModel() {
var emailAddress = MutableLiveData<String>()
Copy link

@abesary abesary Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to expose the mutable versions of our LiveData fields to the view i.e. we don't want the view to be able to update them because that violates the direction of data flow. Use instead:

private val mutableEmailAddress = MutableLiveData<String>()
val emailAddress: LiveData<String> = mutableEmailAddress

The view can only observe the immutable reference. Only your vm can update the mutable because it is private.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for all live data fields.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like russell and dz went back and forth on this IntrepidPursuits/skotlinton-android#33. Gonna go with anton's change to the BaseViewModel

super.onCreate(savedInstanceState)
setContentView(R.layout.activity_main)
val fragment = LoginFragment()
supportFragmentManager.beginTransaction().add(R.id.container, fragment).commit()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably be easier to read if you break it down in different lines

const val PASSWORD_LENGTH = 5

data class User(val emailAddress: String, val password: String) {
val isEmailValid: Boolean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine you'd have to do email validation more than just when you get the User object. Probably be more useful in a Utils class.

This if fine for now because it's only used for User class.

android:ems="10"
android:hint="@string/e_mail_address_field_placeholder"
android:inputType="textEmailAddress"
android:text="@={LoginViewModel.EmailAddress}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the capital letter EmailAddress a typo? your field in your view model is small letter e.

android:ems="10"
android:hint="@string/password_field_hint"
android:inputType="textPassword"
android:text="@={LoginViewModel.Password}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

val emailAddress: LiveData<String> = mutableEmailAddress
val password: LiveData<String> = mutablePassword

private var userMutableLiveData: MutableLiveData<User> = MutableLiveData()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val


private var userMutableLiveData: MutableLiveData<User> = MutableLiveData()

val user: MutableLiveData<User>
Copy link

@abesary abesary Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this isn't in the same format as the other live data fields? i.e.

private val mutableUser = MutableLiveData<User>()
val user: LiveData<User> = mutableUser

Copy link

@abesary abesary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few changes but good otherwise

set(value) {
(this as MutableLiveData<T>).value = value
}
} No newline at end of file
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line

private fun setErrorOnField(editText: EditText, errorResource: Int) {
editText.error = getString(errorResource)
editText.requestFocus()
private fun EditText.setErrorOnField(errorType: LoginErrorType) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Sealed class 🔥

const val PASSWORD_LENGTH = 5

@Bindable
sealed class LoginErrorType {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move below LoginViewModel class

android:inputType="textEmailAddress"
android:text="@={LoginViewModel.email}"
android:hint="@string/username_field_placeholder"
android:text="@={LoginViewModel.username}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change it to lower case loginViewModel since it's a variable name.

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.

2 participants