-
Notifications
You must be signed in to change notification settings - Fork 984
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
New composer - add audio comp #15790
Conversation
[quo/record-audio | ||
{:record-audio-permission-granted record-permission? | ||
:on-start-recording (fn [] | ||
(reset! recording? 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.
@briansztamfater I am not sure what was on-init
function doing? I tried removing it and did not notice any issue. Please let me know if it is really 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.
It is used to save the reset function in record-audio-reset-fn
atom from the audio component and reset the state on show-send / hide-send functions in current (old) composer implementation. One of the reasons, for example, let's say a user is replying to a message and recording an audio while the keyboard is being shown. Then, in the middle of the recording, the user starts typing a message, causing the the recorder to be hidden and text with the send button to be shown, so @record-audio-reset-fn
is called to stop the recording and reset current audio component to its initial state.
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.
Got it! Thank you @briansztamfater
Hi @briansztamfater, |
Jenkins BuildsClick to see older builds (11)
|
Not that I am aware of, I tested those cases carefully while initially integrating the component so maybe something broke, I will check. |
@OmarBasem Please cherry-pick e4d7b2828 and let me know if that fixes the issue |
9350bc3
to
d773fd2
Compare
✔️ status-mobile/prs/tests/PR-15790#2 🔹 ~6 min 51 sec 🔹 d773fd2 🔹 📦 tests package |
Perfect! |
lint rever pod review lint review udpates updates updates udpates updates updates lint updates composer shell button updates updates updates updates updates updates updates updates
3ef71e0
to
d775438
Compare
73% of end-end tests have passed
Failed tests (8)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Passed tests (22)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
|
Hello @omar! I was wondering if you could kindly clarify why the manual testing phase was skipped before merging this PR? Thank you for your assistance. |
Hi @pavloburykh, the new composer is still a work in progress and is still disabled on |
Got it! Thanx for the explanation @OmarBasem |
fixes: #15729