-
Notifications
You must be signed in to change notification settings - Fork 155
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
Show correct UI when replying to a voice message #1658
Conversation
6b8dfba
to
142a17a
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.
Thanks, LGTM, will approve after the screenshots have been recorded.
), | ||
anActionListState().copy( | ||
target = ActionListState.Target.Success( | ||
event = aTimelineItemEvent(content = aTimelineItemVoiceContent()).copy( |
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.
There is no case with aTimelineItemFileContent
anymore?
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.
whooops :) fixed
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.
Though I was also suprised there wasn't a aTimelineItemAudioContent
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 adding them all!
is AudioMessageType -> { | ||
when (type.isVoiceMessage) { | ||
true -> true | ||
false -> true |
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.
Always true
then?
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.
In the future we'll have to split AudioMessageType
in two (AudioMessageType
and VoiceMessageType
).
When we do this having such structure in place might help a bit.
What do you think?
Should I actually split the message type right now?
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 worries, if there are plan to change this later :)
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1658 +/- ##
===========================================
+ Coverage 63.03% 63.08% +0.05%
===========================================
Files 1219 1219
Lines 31389 31420 +31
Branches 6450 6455 +5
===========================================
+ Hits 19786 19822 +36
+ Misses 8630 8625 -5
Partials 2973 2973
☔ View full report in Codecov by Sentry. |
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.
Since @bmarty already gave his approval, I'll approve it now we have some screenshots. There are some missing for the room list and timeline, but we can add them in a separate PR.
Shows voice messages in the room summary.
Shows voice messages in the reply context menu and composer.
Show replies to voice messages in the timeline.
(before this PR voice messages were shown the same as audio messages)
Story: element-hq/element-meta#2106