-
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
feat: edit/delete/reply for emoji/sticker/audio #13808
Conversation
Jenkins BuildsClick to see older builds (53)
|
To fix all the unnecessary formatting changes caused by the IDE, I had to open a new PR. Please follow on this PR. @flexsurfer @rasom @J-Son89 @briansztamfater @Parveshdhull |
(letsubs [contact-name [:contacts/contact-name-by-identity from] | ||
replied-message (@(re-frame/subscribe [:chats/chat-messages chat-id]) id)] ;; message replied to |
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.
As was suggested in the previous PR it makes sense to replacedefview
with defn and letsubs
with let
. defview
is straightforward, adding a sample for letsubs
:
(let [contact-name @(re-frame/subscribe [:contacts/contact-name-by-identity from]])
replied-message (get @(re-frame/subscribe [:chats/chat-messages chat-id]) id)]
...
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.
Aha got it, thanks!
a0ef7da
to
6581d2a
Compare
i believe there was a status-go change in the first PR ? |
:id :reply | ||
:label (i18n/label :t/message-reply)}] | ||
(when (and outgoing config/delete-message-enabled?)) | ||
[{:on-press #(re-frame/dispatch [:chat.ui/soft-delete-message message]) |
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.
[{:on-press..
should be inside when form
@flexsurfer Yes, as mentioned above, this PR depends on that status-go PR for backend functionality. Commit hash: 87db7d953e8fc748baa1fc56a7adffd8fada3055 |
I don't understand why it cannot be built now (for iOS) after rebasing.
|
hey @OmarBasem sorry for late response, could you please rebase, and try |
@flexsurfer As mentioned above, I have already tried rebasing 3 times, but iOS build kept failing. |
Hi @OmarBasem, there was problem with ios build, now fixed in #13912 Also, looks like other failed builds are due to the issue mentioned in #13620 (comment) . This one is also fixed in #13926 Please can you try rebase now, builds should work fine. |
hey @OmarBasem you need to update status go version |
I see all checks have passed. It is the same version v0.107.1. |
100% of end-end tests have passed
Passed tests (87)Click to expandClass TestPublicChatMultipleDeviceMerged:
Class TestRestoreOneDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestSendTxDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestWalletManagementDeviceMerged:
|
but I suppose this PR depends on status-im/status-go#2771 ? so version from PR should be used? |
@flexsurfer That's what I had when I first made the PR, but it was causing merge conflicts. Shall I update it now? |
yes, please |
@flexsurfer is |
i think yes |
Apparently not. The develop branch is currently using an older commit. I updated it to the corresponding commit. |
hey @OmarBasem please fix |
@flexsurfer What do you mean can be tested in the new UI? This PR is implementing old UI functionalities |
ok , @OmarBasem lets then merge it, because we don't test old ui anymore |
but you need to bump version in status-go, so we can use it |
Yes I already did |
status-im/status-go@5069215...938e798 * feat: edit/delete/reply, fixed unnecessary changes
This PR is a continuation of that PR
Fixes #13246
Summary
Add missing actions (edit/delete/reply) to certain types of messages (emoji/sticker/audio)
To implement the needed functionalities there are 2 pull requests to this repo
status-mobile
and status-go.Editing emoji messages: Firstly, I had to allow the emoji content type to be edited from status-go. Secondly, when editing an emoji message, I had to check whether it is still an emoji message or is now a text message, and send the contentType. Also, in status-go I added a "ContentType" field to the "EditMessage" struct and updated the content type when a message is edited. Regarding UI, I had to make sure a white space is added at the end of an emoji message when edited (similar to text messages) to avoid overlapping with the "edited" text and icon.
Deleting emoji messages: added delete option on-long-press.
Audio message reply: In status-go, I added an AudioLocalURL field to the QuotedMessage struct. When an audio message is replied to, the AudioLocalURL field will be assigned to the QuotedMessage. In status-mobile, inside the quoted-message method, I check if the quoted message contains audio and if so, will render the audio message.
Sticker message reply: Initially, I attempted to do it in a similar way to replying to audio, but it didn't work as the sticker url is created using the sticker hash and not the message ID. Currently, the "responseTo" field includes the messageID only. So, what I did is simply get the sticker url from the message itself using the messageID inside the quoted-message method in status-mobile. I added a
HasSticker
field to the QuotedMessage struct in status-go to know if it is a sticker. Additionally, if the sticker is deleted, a message will be shown to the user stating "This message has been deleted!", which I think can also be implemented for replying to other content types.Sticker message delete: added delete option on-long-press.
Sticker reply and delete also work in public chats.
Review Notes
This PR depends on that status-go PR for backend functionality: 87db7d953e8fc748baa1fc56a7adffd8fada3055
Platforms
status: ready