-
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
fixed overlapping last message by the composer when there is a pinned… #20606
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (68)
|
79f3b31
to
e37afa0
Compare
ea97a4f
to
605355a
Compare
21839ae
to
d9feecc
Compare
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.
@jo-mut, I tried to reproduce the issue as you can see from the screenshots. Apparently this bug doesn't always happen because I couldn't reproduce, but I'm sure it's there :)
But more importantly, the solution in this PR introduces a side-effect, the bottom margin is always being applied. There's probably a way to avoid this side-effect.
See screenshots, with or without pinned messages:
@@ -401,8 +401,11 @@ | |||
{window-height :height} (rn/get-window) | |||
context (rf/sub [:chats/current-chat-message-list-view-context]) | |||
messages (rf/sub [:chats/raw-chat-messages-stream chat-id]) | |||
pinned (rf/sub [:chats/pinned-sorted-list chat-id]) | |||
margin-bottom (and community-channel? (not (nil? pinned))) |
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.
Possibly just (not pinned)
is sufficient.
recording? (rf/sub [:chats/recording?])] | ||
[rn/view {:style {:flex 3}} ;; Pushes composer to bottom | ||
[rn/view | ||
{:style (style/permission-context-sheet margin-bottom)} |
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.
margin-bottom
needs a question mark because it's a boolean
d9feecc
to
d5258da
Compare
@ilmotta I have made some updates on this, so instead of checking on pinned messages, we instead check on if the user can post to community and if its a community chat. Screen.Recording.2024-07-19.at.17.19.28.mov |
@ilmotta something else I am noticing, if there is a pinned message and the message is unpinned, the app is killed and restarted and navigate to the current chat the message is still pinned. this behavior is inconsistent maybe its because i was using an old account or something. I am not reproducing it any more |
d5258da
to
d6096ec
Compare
Thanks @jo-mut. I'll check the PR again later today.
There's a delay for messages to propagate in the network. Maybe because of that, when you unpin and immediately close the app, when reopening, the deletion may not have fully propagated yet? Or do you mean you reopen and the message is never unpinned? |
I am also closing the following issue #20779, I think its because was using an old account. |
yeah, when I pin and unpin a message and restart the app, the message is still pinned but not its not reproducible, so maybe its because of message reliablity issues or could because i was using an old account in testing |
@ilmotta this is what I meant before, when there is a pinned message and its unpinned, on-reopening the community channel the message is still pinned and when the message is not pinned and I pin the message on-reopening the channel the message in not pinned. The behaviour is the opposite when the channel is closed and reopened. It could be message reliability Screen.Recording.2024-07-23.at.10.54.17.movScreen.Recording.2024-07-23.at.10.53.43.mov |
@jo-mut, in this example, I think the user can't unpin a message if they are not a member, that's why you see the message is still unpinned. If they could, anybody would be able to alter the list of pinned messages. The actual bug is that the user shouldn't be able to pin/unpin messages if they are not a member. This we should fix if possible. |
@ilmotta got you, I think that can be handled in a separate issues, it falls out of scope for this issue. As for this pr, the overlapping of the last-message is fixed |
shall I create an issue or have you already done so? I think we can do this one in this release |
d6096ec
to
ca53173
Compare
@ilmotta I added a commit to prevent a user who is not a member from pinning and un-pinning channel messages, I thinks its a small issue that can be bundled up in this pr. Once approved I will let the QA know so they can take this into account when testing. |
b753ce9
to
d46a9bb
Compare
@ilmotta I made last minute changes to check if user is a community member instead of using |
57% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
|
Thanks @jo-mut. Let's also see the feedback from a QA about this point. I don't know the granularity at which pinning messages are controllable. Double-checking how the desktop app behaves is another sure way to verify how it should be or pinging a desktop dev. Being a community member may be correct, but might be too permissive too and still not the precise check we should do because the user is doing an operation at the channel level, not community level. |
53% of end-end tests have passed
Failed tests (23)Click to expandClass TestActivityMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
3261ac0
to
ced0506
Compare
ced0506
to
df32b17
Compare
Hi @ilmotta ! This PR touches some areas needed to be retested. Sorry for the delay.
![]()
![]() and it is the way for user to be a member but NOT to have ability to post (so NOT being able to send a message) But this functionality should be retested as there are issues in token gated functionality and I can not clearly say will this |
@ilmotta will leave this PR in Testing status until 2.31 if you do not mind as we are not planning to include it in 2.30 |
100% of end-end tests have passed
Passed tests (7)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
Agreed @mariia-skrypnyk. Better that way 👍🏼 |
also better to mind composer simlification, might be also affected #20125 |
It depends if the work in this PR is mostly done, if it is, we can just update/rebase and QA it. @mariia-skrypnyk You mentioned before |
I agree that in general pin logic is outside of scope of the PR; |
df32b17
to
92c3876
Compare
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Passed tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
Hi @ilmotta @ulisesmac ! Rebased PR and retested on iOS and Android. |
fixes
#20513
Summary
This pr adds margin-bottom when the user is not allowed to post a message in a community channel because they are not a member. The margin-bottom is the height of the permission bottom sheet that is shown when the user is no longer a community member
A note to @status-im/mobile-qa, this pr also fixes a bug explained in this comment #20606 (comment). Its a small simple fix I bundled up in this pr
Before and after screenshots comparison
status: ready