Improve last message preview #15613
Improve last message preview #15613
Conversation
Jenkins BuildsClick to see older builds (84)
|
vkjr
force-pushed
the
messages_preview_issues
branch
2 times, most recently
from
b99dd46
to
7a5850a
Compare
vkjr
force-pushed
the
messages_preview_issues
branch
3 times, most recently
from
f531449
to
3955eec
Compare
Makefile
Outdated
| @@ -273,7 +273,7 @@ run-android: export TARGET := android | |||
| run-android: ##@run Build Android APK and start it on the device | |||
| npx react-native run-android --appIdSuffix debug | |||
|
|
|||
| SIMULATOR=iPhone 13 | |||
| SIMULATOR=iPhone 14 | |||
There was a problem hiding this comment.
I upgraded to the latest macOS and latest XCode and now my system complains that I don't have iPhone 13, so I made a local change to Makefile and forgot to remove it)
Btw, what iPhone version do you use on the simulator? Wondering if a lot of us have similar local changes locally, maybe we can make it to a Makefile.
There was a problem hiding this comment.
we all should use iPhone 11 Pro because it has the same size as Figma designs but I usually use make run-ios SIMULATOR="iPhone 11 Pro" btw you can always add any version of simulator, im wondering how this SIMULATOR value is used ?
There was a problem hiding this comment.
It is used to set the version if it is not provided directly, I believe:
ifneq ("$(SIMULATOR)", "")
npx react-native run-ios --simulator="$(SIMULATOR)"
else
| @@ -1,12 +1,15 @@ | |||
| (ns status-im2.contexts.chat.home.chat-list-item.view | |||
| (:require [clojure.string :as string] | |||
| [quo2.core :as quo] | |||
| (:require [quo2.core :as quo] | |||
| [quo2.foundations.colors :as colors] | |||
| [react-native.core :as rn] | |||
| [utils.datetime :as datetime] | |||
| [status-im2.common.home.actions.view :as actions] ;;TODO move to status-im2 | |||
There was a problem hiding this comment.
could you pls remove ;;TODO move to status-im2
| (reagent/create-class | ||
| {:component-did-mount | ||
| (fn [_] | ||
| (rf/dispatch [:pin-message/load-pin-messages chat-id])) |
There was a problem hiding this comment.
could you pls elaborate on why we want this here and only once when mount?
There was a problem hiding this comment.
We need the details of the pinned message to extract text for message preview. But they are loaded only when chat opened, so I've added this call, to get them earlier. And I wanted to reduce the number of calls in case of re-rendering, but after your question I think maybe it is worth calling on every re-render because it can be caused because the last message changed. Wdyt?
There was a problem hiding this comment.
@vkjr, calling once seems alright to me if the last message will change anyway due to signals emitted by status-go, or isn't it the case?
There was a problem hiding this comment.
I'm wondering why it isn't a part of the last message itself? it looks wrong and not performant to load pin messages for all chats
There was a problem hiding this comment.
@ilmotta, appears that it is better to be called on render because while the messages list is updated by signal, the pinned messages don't and that causes issue.
@flexsurfer, agree, let me check if I'll be able to pass the whole last message along with the chat preview from status-go instead of separate fields.
There was a problem hiding this comment.
we changed this because of performance, we don't want an entire message, sometimes it's too heavy, so we just need to add fields we need
| (let [{:keys [parsed-text text]} content] | ||
| (if parsed-text (parsed-text-to-one-line parsed-text) text))) | ||
|
|
||
|
|
| (str sent-photos ": " content-text) | ||
| sent-photos)) | ||
|
|
||
|
|
| (defn last-message-preview | ||
| "Render the preview of a last message to a maximum of max-subheader-length characters" | ||
| [chat-id _] | ||
| (reagent/create-class |
There was a problem hiding this comment.
If possible, it'd be good if we could use hooks, since we can achieve the same results without the React lifecycle methods.
vkjr
force-pushed
the
messages_preview_issues
branch
from
6fbd5f5
to
5f7e42f
Compare
| "Render the preview of a last message to a maximum of max-subheader-length characters" | ||
| [chat-id {:keys [content-type content outgoing album-images-count] :as message}] | ||
|
|
||
| (rf/dispatch [:pin-message/load-pin-messages chat-id]) |
There was a problem hiding this comment.
mmh, this does not look right, we should not be dispatching in a render, why do you need to do this exactly? probably we can avoid doing this by returning the data you need directly from status-go
| (str (i18n/label :t/Pinned) " " pinned-content-text)) | ||
| (if outgoing | ||
| (i18n/label :t/you-pinned-a-message) | ||
| (i18n/label :t/Pinned-a-message)))) |
There was a problem hiding this comment.
capital P, I think we use lowercase for keys, not sure if we want to change the behavior
There was a problem hiding this comment.
We already had a few similar translations which are different only in capitalized first letter, so I followed similar pattern , because we already have translation for "pinned messages" but with lowercase "p".
|
I see what the issue is, @vkjr I'd suggest descoping full text pinned messages preview for now, until we find a better solution, for now, I'd go for the case when there's no text, so you don't need to subscribe/load, and if you don't mind we can create an issue and address separately, what do you think? |
Sure, thanks! |
|
Separate issue #15683 created for the preview of pinned messages |
vkjr
force-pushed
the
messages_preview_issues
branch
from
3f06fac
to
6efa292
Compare
90% of end-end tests have passedFailed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (26)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@qoqobolo, thank you for checking! As far as I get, groups weren't in the original scope of the issue, it was all about chats. So I suspect we can move your finding in a separate issue, wdyt? |
|
@vkjr as I can see in the description of the original issue for both issue 1 and issue 2, step 1 is: |
|
@qoqobolo, you are right, I missed groups in steps) Will fix it here |
vkjr
force-pushed
the
messages_preview_issues
branch
from
6efa292
to
e90c80b
Compare
|
@vkjr cool thank you, will check shortly! |
vkjr
force-pushed
the
messages_preview_issues
branch
from
6309626
to
c6a31a1
Compare
|
@qoqobolo, thanks! Rebased. |
|
Thanks @vkjr! ISSUE 2: RepliesThe word Expected result: 1-1 chat Group Actual result: 1-1 chat Group ISSUE 3: MentionsWe should display Expected result: 1-1 chat Group Actual result: 1-1 and group
ISSUE 4: Pinned messagesWe should display the content of the pinned message Expected result: 1-1 chat Group Actual result: 1-1 and group ISSUE 5: Deleted messagesA system message should be displayed Expected result: Group Actual result: ISSUE 6: The user name is deleted from the preview if the message was edited in group chatsSteps:
IMG_1421.MP4ISSUE 7: Nicknames are not displayed in a group chat previewSteps:
IMG_1417.MP4 |
Ah sorry! So carried away that I forgot about it, although I saw the comments above 😅 |
vkjr
force-pushed
the
messages_preview_issues
branch
3 times, most recently
from
65a3683
to
41ff397
Compare
|
@qoqobolo, the issues were fixed, could you please recheck when have time? Thanks! |
81% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (25)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
Thanks for the great work @vkjr! |
vkjr
force-pushed
the
messages_preview_issues
branch
from
41ff397
to
e405d38
Compare
vkjr
force-pushed
the
messages_preview_issues
branch
from
e405d38
to
b3e8ed4
Compare
fixes #15281, #14543
Summary
The last message preview was extended to generate correct preview text for: photos, audio, pinned messages, audio, etc.
Related
status-goPR extended chat preview structure with more fields to make previews aligned with designs.Review notes
Testing notes
status: ready