-
Notifications
You must be signed in to change notification settings - Fork 296
Add thread decoration with latest event details #5355
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 thread decoration with latest event details #5355
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5355 +/- ##
===========================================
- Coverage 80.38% 80.37% -0.02%
===========================================
Files 2279 2282 +3
Lines 62961 63068 +107
Branches 7876 7887 +11
===========================================
+ Hits 50614 50691 +77
- Misses 9442 9464 +22
- Partials 2905 2913 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fcf902e
to
daead53
Compare
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 works pretty well, thanks!
A few remarks in order to simplify (I think!) the code though.
...kotlin/io/element/android/features/messages/impl/timeline/components/TimelineItemEventRow.kt
Outdated
Show resolved
Hide resolved
Spacer(modifier = Modifier.width(8.dp)) | ||
|
||
Text( | ||
text = latestEvent.senderProfile.getDisplayName() ?: latestEvent.senderId.value, |
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.
We should use the disambiguated displayname here:
text = latestEvent.senderProfile.getDisplayName() ?: latestEvent.senderId.value, | |
text = latestEvent.senderProfile.getDisambiguatedDisplayName(latestEvent.senderId), |
.../element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt
Show resolved
Hide resolved
.../element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt
Outdated
Show resolved
Hide resolved
...mpl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/TimelineItem.kt
Outdated
Show resolved
Hide resolved
daead53
to
236c1b0
Compare
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.
val threadId = targetEvent.threadInfo.threadRootId ?: targetEvent.eventId!!.toThreadId() | ||
val threadId = when (targetEvent.threadInfo) { | ||
is TimelineItemThreadInfo.ThreadResponse -> targetEvent.threadInfo.threadRootId | ||
is TimelineItemThreadInfo.ThreadRoot, null -> targetEvent.eventId?.toThreadId() |
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.
Not a big deal but since we are not using the smart cast on TimelineItemThreadInfo.ThreadRoot
(I was confused about that at the beginning), maybe using else
is cleaner.
is TimelineItemThreadInfo.ThreadRoot, null -> targetEvent.eventId?.toThreadId() | |
else -> targetEvent.eventId?.toThreadId() |
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 wanted to explicitly list all the possible cases so if we end up adding one in the future (unlikely, but...) we'll have to come back here to manage it.
...mpl/src/main/kotlin/io/element/android/features/messages/impl/timeline/model/TimelineItem.kt
Outdated
Show resolved
Hide resolved
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!
# Conflicts: # features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/actionlist/ActionListPresenterTest.kt
7eff9c9
to
7f8b5bc
Compare
- Thread icon should be 20x20. - Corner radius should be 8.
|
Content
MessageSummaryFormatter
to create the summary based on the event content, not the whole event. This is needed because the thread summary info only contains the content.TimelineItemEventFactory
calculate the text summary from the latest event from the thread.ThreadSummaryView
.MessageBubbleDefaults
so they can be reused in that previous view.Motivation and context
Implements the Android part of the designs for element-hq/element-meta#2771.
Screenshots / GIFs
In the PR.
Tests
Enable the 'Threads' feature flag, the navigate to a room with threads. Check the info from the summary is displayed correctly.
Tested devices
Checklist