-
Notifications
You must be signed in to change notification settings - Fork 868
empty state for new invites screen #6986
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
Conversation
vector/src/main/java/im/vector/app/features/home/room/list/home/invites/InvitesViewModel.kt
Outdated
Show resolved
Hide resolved
| } | ||
| pagedList.asFlow() | ||
| .map { | ||
| if (it.isEmpty()) { |
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.
out of interest, is an empty result only returned after the pagedList has finished loading?
or is it possible for it to emit an empty state instantly?
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.
up to my tests it emits empty list only when there are no invites
| is InvitesContentState.Content -> { | ||
| views.invitesStateView.state = StateView.State.Content | ||
| Suppress("UNCHECKED_CAST") | ||
| controller.submitList(it.content as? PagedList<RoomSummary>) |
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.
is this cast (and suppress) needed?
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.
no, it's a leftover from attempt to make solution more generic
bmarty
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.
LGTM, some minor remarks though.
vector/src/main/java/im/vector/app/features/home/room/list/home/invites/InvitesViewModel.kt
Outdated
Show resolved
Hide resolved
| message = stringProvider.getString(R.string.invites_empty_message) | ||
| ) | ||
| } else { | ||
| invitesCount = it.loadedCount |
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 will work since we will test only if this is equal to 1, but I think it's more correct to use it.size()
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.
it.size() will also return placeholders (if there are any), while here we are interested in exact count without them
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.
from my understanding, the exact count is returned by size(). loadedCount does not include placeholders (= not loaded data, but which exist). If we have for instance 120 invites, there is maybe a risk that loadedCount return 0, or 1 (and so we have a bug), while size() returns 120, during the time the data are not yet loaded.
Anyway the risk is very low I think, and probably temporary, since more data will be loaded to be displayed.
vector/src/main/java/im/vector/app/features/home/room/list/home/invites/InvitesViewState.kt
Outdated
Show resolved
Hide resolved
| android:fastScrollEnabled="true" | ||
| android:overScrollMode="always" | ||
| android:scrollbars="vertical" | ||
| app:layout_behavior="@string/appbar_scrolling_view_behavior" |
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.
To double check, is it OK to remove this layout_behavior?
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.
yes, we don't need it for state view
|
SonarCloud Quality Gate failed. |
|
Test |
bmarty
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.
LGTM








Type of change
Content
new invites screen should also present empty state
Motivation and context
closes #6876
part of #6502
Screenshots / GIFs