[#17207] fix: incorrect mentions alignment in messages #17212
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
mohsen-ghafouri
requested review from
ilmotta,
flexsurfer,
ibrkhalil and
siddarthkay
Jenkins BuildsClick to see older builds (15)
|
cammellos
reviewed
Sep 7, 2023
mohsen-ghafouri
force-pushed
the
fix/incorrect-mention-alignment
branch
2 times, most recently
from
bcb6db3 to
bed5aea
Compare
flexsurfer
reviewed
Sep 7, 2023
| :size :paragraph-1} | ||
| (rf/sub [:messages/resolve-mention literal])]]]) | ||
| [rn/touchable-opacity | ||
| {:active-opacity 1 |
There was a problem hiding this comment.
It was set this way in the past, and I believe the reason is that we don't require an opacity effect when a user clicks on a mention.
There was a problem hiding this comment.
@OmarBasem can you remember any reason for set active-opacity or i can remove it? i believe it was part of this PR #15799
There was a problem hiding this comment.
:active-opacity 1 is not redundant as the default active-opacity is 0.5. However, we are moving away from touchable-opacity, so you can replace it with rn/pressable and remove active-opacity
flexsurfer
approved these changes
Sep 7, 2023
77% of end-end tests have passedFailed tests (10)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (33)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
|
mohsen-ghafouri
force-pushed
the
fix/incorrect-mention-alignment
branch
from
bed5aea to
405ec1e
Compare
|
@mohsen-ghafouri thanx for the PR! Alignment is much better now. Though I still facing alignment issues which are more visible on IOS in my case. IOS 16.6, iPhone X:
Android 12, Samsung Galaxy A52:
|
|
@Francesca-G Thank you in advance! |
|
thanks @pavloburykh, as i said it involves trial and error to adjust the position, let me do another round with iPhone X |
Francesca-G
approved these changes
Sep 7, 2023
There was a problem hiding this comment.
I haven't experienced huge differences between this PR and nightly @churik but if the fixes made are to prevent misalignment that may occur with different names and mentions I think we should stick to it.
Even if I didn't have any incorrect alignment, the main issue (present in both) is the spacing between the two mentions when they're in the same 2-line text
Thanx @mohsen-ghafouri! We decided that we are ready to merge current PR as it is, as we need it ASAP for the branch cut. Let's handle these issues in a separate PR. I will create followup once this PR is merged. |
|
@pavloburykh just a few minutes i will push a new update and you can test it for the last time :) |
mohsen-ghafouri
force-pushed
the
fix/incorrect-mention-alignment
branch
from
405ec1e to
4ea37ca
Compare
|
@pavloburykh done, please check again. tests from my POV Android Pixel XL Android 12 |
siddarthkay
approved these changes
Sep 7, 2023
|
@Francesca-G I believe the issue on Android related to the space between mention tags should be improved by the latest changes. Please check my screenshot. |
|
@mohsen-ghafouri thank you! Looks much more better now. Couple of issues will be logged as followup:
Let's merge this PR. Nice work! |
63% of end-end tests have passedFailed tests (16)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Passed tests (27)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
|
mohsen-ghafouri
force-pushed
the
fix/incorrect-mention-alignment
branch
from
4ea37ca to
90faafd
Compare
mohsen-ghafouri
force-pushed
the
fix/incorrect-mention-alignment
branch
from
90faafd to
1439590
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 #17207
I've noticed some issues related to the mention tag, and I'd like to provide a technical explanation that we can refer to in case we encounter similar problems in the future.
Issues:
The mention tag faces certain limitations because we're rendering a view inside a text component. Essentially, it's affected by an open issue in React Native regarding vertical alignment (Nested View and Text vertical alignment issue).
The workaround we currently employ involves trial and error to adjust the position of the mention tag so that it aligns with the text. Please note that this alignment may not be consistent across all OS versions and platforms.
Additionally, I explored an alternative solution that involves using just a text component instead of a view. However, this approach also has its limitations, particularly related to styles. For instance, nested text components don't accept properties like border radius or padding, although alignment works correctly.
So what I've done here is applied some additional improvements until the React Native team addresses the issue, or until we discover an alternative workaround or a different design approach that's implementable.
cc @pavloburykh @cammellos
Android Before -> After
iOS Before -> After
status: ready