[#18554] incorrect drawer for non community members #21597
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 (8)
|
ulisesmac
changed the title
|
idk probably it's better to avoid formatting changes, it's much harder to review |
flexsurfer
approved these changes
Nov 7, 2024
88% of end-end tests have passedExpected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
Parveshdhull
approved these changes
Nov 7, 2024
Sorry, I forgot to add a self review, I'll add it now |
ulisesmac
commented
Nov 7, 2024
Comment on lines
102
to
126
| (let [edit-message? (and community-member? | ||
| outgoing | ||
| (not (or deleted? deleted-for-me?)) | ||
| ;; temporarily disable edit image message until | ||
| ;; https://github.com/status-im/status-mobile/issues/15298 is | ||
| ;; implemented | ||
| (not= content-type constants/content-type-image) | ||
| (not= content-type constants/content-type-audio)) | ||
| reply? (and able-to-send-message? | ||
| (not= outgoing-status :sending) | ||
| (not (or deleted? deleted-for-me?))) | ||
| copy-text? (and (not (or deleted? deleted-for-me?)) | ||
| (not= content-type constants/content-type-audio)) | ||
| ;; pinning images are temporarily disabled | ||
| pin-message? (and message-pin-enabled | ||
| (not= content-type constants/content-type-image) | ||
| (or community-member? (not community?))) | ||
| delete-for-me? (and community-member? | ||
| (not (or deleted? deleted-for-me? bridge-message))) | ||
| delete-for-everyone? (and community-member? | ||
| (or (not deleted?) | ||
| outgoing | ||
| (and community? can-delete-message-for-everyone?) | ||
| (and group-chat group-admin?) | ||
| (not bridge-message)))] |
There was a problem hiding this comment.
Modified this logic to properly handle the shown options
ulisesmac
commented
Nov 7, 2024
| (and community? can-delete-message-for-everyone?) | ||
| (and group-chat group-admin?) | ||
| (not bridge-message)))] | ||
| (cond-> [] |
There was a problem hiding this comment.
changed:
(concat
(when something? [:a])
(when foo? [:b])
(when bar? [:c]))to:
(cond-> []
something? (conj :a)
foo? (conj :b))
ulisesmac
commented
Nov 7, 2024
Comment on lines
241
to
246
| (when (and (or community-member?) | ||
| (not= outgoing-status :sending) | ||
| (not (or deleted? deleted-for-me?))) | ||
| [reactions {:chat-id chat-id :message-id message-id}]) |
There was a problem hiding this comment.
Needed to limit the reactions to only community members
ulisesmac
commented
Nov 7, 2024
Comment on lines
8
to
12
| :bottom (+ (safe-area/get-bottom) | ||
| (when-not able-to-send-messages? 46) | ||
| shell.constants/floating-shell-button-height)}) |
There was a problem hiding this comment.
This is the second fix about the scroll to bottom button in chats
There was a problem hiding this comment.
This 46 is suspicious @ulisesmac, do you know if it can be calculated to be the sum of other known values?
There was a problem hiding this comment.
Thanks for noticing @ilmotta
This is the height of the bottom drawer that says "requst to join". It's specified in figma. I can name it, do you think that'd be better?
There was a problem hiding this comment.
It's a large and specific number @ulisesmac, that's why it caught my attention. Usually if it's 4, 8, 12, 20 we know they are just standard spacing values, but 46 I thought could be a magic number. Numbers that are tied to a specific component width/height are usually good candidates be defined as vars. One reason is that if designers change in Figma we won't be able to easily/safely adjust in the code.
ilmotta
approved these changes
Nov 7, 2024
Comment on lines
8
to
12
| :bottom (+ (safe-area/get-bottom) | ||
| (when-not able-to-send-messages? 46) | ||
| shell.constants/floating-shell-button-height)}) |
There was a problem hiding this comment.
This 46 is suspicious @ulisesmac, do you know if it can be calculated to be the sum of other known values?
|
Hey @ulisesmac ! Is PR ready for QA? If yes could you please add |
|
@ulisesmac thanks for the PR ISSUE 1 group/1-1 chats drawer missing optionsActual result: 1:1 chat
other's messages drawer is missing following options:
group chat
other's messages drawer is missing following options:
Expected result: someone's message drawer: |
|
@pavloburykh Thank you for testing it, I'll solve them, I forgot to check the implementation for 1:1 chats. |
ulisesmac
force-pushed
the
18554-incorrect-drawer-for-non-members-community
branch
2 times, most recently
from
2751cc8
to
d08b375
Compare
|
@pavloburykh fixed. Could you please test it again? BTW, I also fixed the order of the items showed, according to figma, we should display:
But we show:
So it's fixed too. |
0% of end-end tests have passedFailed tests (35)Click to expandClass TestFallbackMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (20)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
|
pavloburykh
force-pushed
the
18554-incorrect-drawer-for-non-members-community
branch
from
ad12661
to
88e209d
Compare
0% of end-end tests have passedFailed tests (35)Click to expandClass TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestFallbackMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletMultipleDevice:
Expected to fail tests (20)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
|
|
@ulisesmac thanks for the PR. Issues are fixed, failed e2e are not PR related. Ready for merge. |
ulisesmac
deleted the
18554-incorrect-drawer-for-non-members-community
branch
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 #21384
fixes #18554
Summary
This PR fixes the modal shown for non-members that sent a message in a community in a chat:
video_2024-11-06_13-50-34.mp4
There are some missing options, such as "forward", but they haven't been implemented yet.
Addintionally fixes:
iOS | Android
Platforms
status: ready