-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[Jetchat] Moves to Scaffold and Compose drawer content #263
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
[Jetchat] Moves to Scaffold and Compose drawer content #263
Conversation
Change-Id: I18d6c90b947fc2cc98c7d2c2683f1954ace200ed
Change-Id: I10a769b54254f94ff67bf9905bc1b736bd6d1b1f
Change-Id: I7f2adb8970cba9f2a91467367e4e39328c609771
Change-Id: Ibad4143a1765e3418ddbda1f8bfdcfc7bf42d63d
Jetchat/app/src/main/java/com/example/compose/jetchat/MainViewModel.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/NavActivity.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/NavActivity.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/NavActivity.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/NavActivity.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/NavActivity.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/components/JetchatDrawer.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/components/JetchatDrawer.kt
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/components/JetchatDrawer.kt
Outdated
Show resolved
Hide resolved
Jetchat/app/src/main/java/com/example/compose/jetchat/conversation/BackHandler.kt
Outdated
Show resolved
Hide resolved
Change-Id: I2e9becb5f7afaecc936f70b98c34f8b803eb847e
manuelvicnt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the changes! This looks great
Added a comment about the usage of StateFlow. Thanks!
|
|
||
| private val _userData = MutableLiveData<ProfileScreenState>() | ||
| val userData: LiveData<ProfileScreenState> = _userData | ||
| private val _userData = MutableStateFlow<ProfileScreenState?>(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is very common when migrating from LD to StateFlow but I'd discourage having a StateFlow of a nullable type. What about creating an empty ProfileScreenState for this?
| private val _userData = MutableStateFlow<ProfileScreenState?>(null) | |
| private val _userData = MutableStateFlow<ProfileScreenState>( | |
| ProfileScreenState("", "", "", "", "") // Empty user to begin with | |
| ) |
The view then should check userId.isEmpty() for showing the error state, or even better, have that function as an extension function on ProfileScreenState to indicate if it's the object is properly constructed or not.
The benefits of doing this might not be obvious for this example, but it's the right pattern to use. In this way, you avoid that you can emit null again for this StateFlow and handling nullability when collecting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're trying to avoid null too much, this is an acceptable case for it. An empty screen state doesn't mean the same thing as null. In the view I can do:
if (userData == null) {
ProfileError()
and display better data to the user than an empty profile. Or I would have to create an extension ProfileScreenState.isEmpty() or something similar which is wasteful.
In a real app you would probably have this data wrapped into a resource sealed class. If there's a big problem with nullable types I would do that first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, leave it like this 🤷♂️
No description provided.