Skip to content
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 UI to record a voice message from the composer toolbar #1892

Merged
merged 15 commits into from
Oct 16, 2023

Conversation

nimau
Copy link
Contributor

@nimau nimau commented Oct 13, 2023

This PR adds the user interface for recording a voice message from the composer toolbar (first part of the story Record and send a voice message)
The recording itself is not yet implemented (but an audio recorder has already been added).

If the feature flag is set, these are the new states of the composer toolbar:

  • Default (the composer text field is empty)
Screenshot 2023-10-13 at 16 19 37
  • Text present in the composer
Screenshot 2023-10-13 at 16 19 57
  • Tap on the voice message button

A tooltip is displayed:
Screenshot 2023-10-13 at 16 17 34

  • Longpress on the voice message button

The composer displays the recording view:
Screenshot 2023-10-13 at 16 20 08

  • Release the voice message button

The composer displays the voice message preview with the send button:
Screenshot 2023-10-13 at 16 20 24

@nimau nimau requested a review from a team as a code owner October 13, 2023 14:23
@nimau nimau requested review from alfogrillo, stefanceriu and pixlwave and removed request for a team and alfogrillo October 13, 2023 14:23
@github-actions
Copy link

github-actions bot commented Oct 13, 2023

Warnings
⚠️ This pull request seems relatively large. Please consider splitting it into multiple smaller ones.
⚠️ Please add a changelog.
⚠️ Some of the commits are missing ticket numbers. Please consider squashing all commits that don't have a tracking number.
⚠️ You seem to have made changes to views. Please consider adding screenshots.

Generated by 🚫 Danger Swift against 36e3fdd

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

Attention: 210 lines in your changes are missing coverage. Please review.

Comparison is base (e42977a) 70.91% compared to head (035ebc3) 70.28%.
Report is 2 commits behind head on develop.

❗ Current head 035ebc3 differs from pull request most recent head 36e3fdd. Consider uploading reports for the commit 36e3fdd to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1892      +/-   ##
===========================================
- Coverage    70.91%   70.28%   -0.63%     
===========================================
  Files          464      473       +9     
  Lines        31468    32228     +760     
  Branches     15355    15705     +350     
===========================================
+ Hits         22316    22653     +337     
- Misses        8588     8992     +404     
- Partials       564      583      +19     
Flag Coverage Δ
unittests 55.10% <64.10%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...creens/ComposerToolbar/ComposerToolbarModels.swift 98.64% <100.00%> (+0.17%) ⬆️
...Screens/ComposerToolbar/View/MessageComposer.swift 90.39% <100.00%> (ø)
.../Sources/Screens/RoomScreen/RoomScreenModels.swift 72.97% <ø> (ø)
...tX/Sources/Services/Audio/Player/AudioPlayer.swift 0.00% <ø> (ø)
...urces/Services/Audio/Player/AudioPlayerState.swift 92.15% <100.00%> (ø)
...entX/Sources/Services/Room/RoomProxyProtocol.swift 68.00% <ø> (ø)
...ineController/RoomTimelineControllerProtocol.swift 0.00% <ø> (ø)
...rvices/VoiceMessage/VoiceMessageMediaManager.swift 86.30% <100.00%> (ø)
...ementX/Sources/Services/Audio/AudioConverter.swift 0.00% <0.00%> (ø)
...serToolbar/View/VoiceMessageRecorderComposer.swift 88.46% <88.46%> (ø)
... and 11 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pixlwave pixlwave removed their request for review October 16, 2023 08:17
Copy link
Member

@stefanceriu stefanceriu left a comment

Choose a reason for hiding this comment

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

Very nice! 👏 Left a couple of comments inline but nothing major.

@nimau nimau requested a review from stefanceriu October 16, 2023 12:21
@nimau nimau force-pushed the nicolas/voice-message-record-ui branch from 98abb34 to 5dddc22 Compare October 16, 2023 14:28
@nimau nimau force-pushed the nicolas/voice-message-record-ui branch from 5dddc22 to 36e3fdd Compare October 16, 2023 14:46
@nimau nimau enabled auto-merge (squash) October 16, 2023 14:47
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@nimau nimau merged commit 9ab3b20 into develop Oct 16, 2023
@nimau nimau deleted the nicolas/voice-message-record-ui branch October 16, 2023 15:18
@jacotec
Copy link

jacotec commented Oct 23, 2023

@nimau I'm missing how to "lock" the recording without the need to keep the recording button pressed all the time. Will it be the "slide up" gesture like in the current Element/IOS?

@nimau
Copy link
Contributor Author

nimau commented Oct 23, 2023

@nimau I'm missing how to "lock" the recording without the need to keep the recording button pressed all the time. Will it be the "slide up" gesture like in the current Element/IOS?

@jacotec, for this first implementation of voice message recording, it is not currently possible to "lock" the recording.

@jacotec
Copy link

jacotec commented Oct 23, 2023

@nimau Hopefully this will be added? It' pretty useless in the car then, much too dangerous to keep the finger all the time on the phone in the holder. Same for longer messages ...

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.

3 participants