Handle long names in system messages #17078
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (51)
|
ajayesivan
force-pushed
the
16345-handle-longer-names-in-messages
branch
3 times, most recently
from
5a12d0c
to
e6fcb8b
Compare
ajayesivan
commented
Aug 31, 2023
| [rn/view | ||
| {:flex-direction :row | ||
| :flex-wrap :wrap | ||
| :height 18.2} |
There was a problem hiding this comment.
@ibrkhalil I've noticed the addition of this height in your PR here. I'm uncertain about its purpose; please advise if its removal could cause any problems.
ajayesivan
commented
Aug 31, 2023
| @@ -70,7 +70,6 @@ | |||
| [rn/view | |||
| {:align-self :center | |||
| :flex-direction :row | |||
| :margin-right 40 ;; dirty hack, flexbox won't work as expected | |||
There was a problem hiding this comment.
@flexsurfer, I've taken out this line and encountered no problems. However, I could be overlooking something. Please review and inform me if its removal triggers any issues.
There was a problem hiding this comment.
i think that's fine, because you've changed the layout of elements and you have :flex-shrink 0
ajayesivan
requested review from
flexsurfer,
ibrkhalil,
BalogunofAfrica,
alwx and
mohsen-ghafouri
flexsurfer
reviewed
Aug 31, 2023
| @@ -70,7 +70,6 @@ | |||
| [rn/view | |||
| {:align-self :center | |||
| :flex-direction :row | |||
| :margin-right 40 ;; dirty hack, flexbox won't work as expected | |||
There was a problem hiding this comment.
i think that's fine, because you've changed the layout of elements and you have :flex-shrink 0
FFFra
reviewed
Aug 31, 2023
| @@ -49,7 +49,7 @@ | |||
| (defn split-text | |||
| [label theme add-pred?] | |||
| (let [color (text-color theme)] | |||
| [:<> | |||
| [rn/view {:style {:flex-direction :row :flex-shrink 0 :align-items :center}} | |||
There was a problem hiding this comment.
If possible, I would suggest moving this to a separate style file
| [rn/view {:flex-direction :row :align-items :center} | ||
| :flex-shrink 1 | ||
| :flex-wrap :nowrap} | ||
| [rn/view {:flex-direction :row :align-items :center :flex-shrink 1} |
| @@ -136,13 +139,16 @@ | |||
| [rn/view | |||
| {:flex-direction :row | |||
| :align-items :center | |||
| :flex-wrap :wrap} | |||
| :flex-shrink 1 | |||
| :flex-wrap :nowrap} | |||
| (when-not incoming? [split-text "Contact request sent to" theme false]) | |||
There was a problem hiding this comment.
Do we have a translation for this string?
| {:weight :semi-bold | ||
| :number-of-lines 1 | ||
| :style {:flex-shrink 1} | ||
| :size :paragraph-2} | ||
| display-name]] | ||
| (when incoming? [split-text "sent you a contact request" theme true])]]) |
There was a problem hiding this comment.
Do we have a translation for this string?
mmilad75
reviewed
Aug 31, 2023
| @@ -70,7 +70,6 @@ | |||
| [rn/view | |||
| {:align-self :center | |||
| :flex-direction :row | |||
There was a problem hiding this comment.
Is there a reason that the styles are in the same file and not in a separate file?
There was a problem hiding this comment.
Other components have a structure like this:
component-name
- view.cljs
- style.cljs
Why this one is not like that?
ajayesivan
force-pushed
the
16345-handle-longer-names-in-messages
branch
from
e6fcb8b
to
3816367
Compare
mohsen-ghafouri
approved these changes
Aug 31, 2023
FFFra
approved these changes
Aug 31, 2023
|
@flexsurfer fixed timestamp alignment 
|
alwx
approved these changes
Aug 31, 2023
|
@ajayesivan i think you PR is also related to 1 comment in this Issue, right? |
60% of end-end tests have passedFailed tests (17)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (26)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
@mohsen-ghafouri This PR only fixes the issue in the system-message component. We have to handle the one in #16867 separately. |
ajayesivan
force-pushed
the
16345-handle-longer-names-in-messages
branch
4 times, most recently
from
405014a
to
99adac0
Compare
|
@VolodLytvynenko Resolved issues 1, 2, and 3. This PR specifically addresses the long name issue within the System Messages component, and issue 4 is unrelated to this component. I think it's more appropriate to address issue 4 in a separate PR, as it appears to be closely related to the first issue in this Design review: #16867. If you think otherwise, please do let me know. |
26% of end-end tests have passedFailed tests (32)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (11)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
churik
force-pushed
the
16345-handle-longer-names-in-messages
branch
from
99adac0
to
23afedf
Compare
63% of end-end tests have passedFailed tests (16)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Passed tests (27)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
72% of end-end tests have passedFailed tests (12)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
Hi, @ajayesivan thank you for the PR and for your patience. I completely agree with you. This issue is my fault. I didn't notice that it already exists on the latest nightly, which means it's not related to this PR. I will report it separately. Thank you! No issues from my side anymore. @Francesca-G kindly, could you please take a look? |
Francesca-G
approved these changes
Sep 6, 2023
There was a problem hiding this comment.
Long names look good :)
Here are a couple of comments all related to the missing avatar in the contact messages
|
I have created a follow-up issue for the missing user avatar issue: #17208 |
ajayesivan
force-pushed
the
16345-handle-longer-names-in-messages
branch
from
23afedf
to
dc295e4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #16345
Summary
Truncate user name in system messages when a user's name is lengthy and cannot be fully displayed along with the system message in a single line.
Review notes
Testing notes
Platforms
Areas that maybe impacted
status: ready