-
Notifications
You must be signed in to change notification settings - Fork 985
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
Show parsed bridge messages #20058
Show parsed bridge messages #20058
Conversation
Jenkins BuildsClick to see older builds (47)
|
dcbbec1
to
0e20aaa
Compare
0e20aaa
to
8615d31
Compare
36d06ef
to
d66ce89
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.
hey @Parveshdhull thank you for the PR, could we please keep it as a separate component and do not mix with our message components
hi @flexsurfer thank you for reviewing PR.
Having separate components do looks neater, but it lacks functionality. We are already duplicating author/message container styles for bridge message, if we are to support press/long-press etc. we have to duplicate this to and more.
Please can you elaborate what will the benefit of having it as separate and what are drawbacks of current approach. |
thank you @Parveshdhull , it should be separate, it's not duplicating, it's just the same implementation, but it's fine that it's still separate, because it's not a regular status message, and it shouldn't behave like status message, so we should keep separate implementation for it, there should be no interactions (like press/long press) etc , adding conditions like |
@pavloburykh Please can you confirm? Because according to issue #19977,
PS: I can revert these changes in PR and only address parsing of bridge message if that's what we want. But If we want to support reactions etc. on bridged message then I think they are working in PR. |
Hey @Parveshdhull! Thank you for this PR.
That's a good point. Let me compare the behaviour on Desktop and I will come back with an answer after that. |
So, I have compared with Desktop app and here what I see. On Desktop user can perform following interactions with Bridged for Discord messages:
On mobile in PR build longtap menu options are slightly different:
Summary:Based on the above comparison I believe we should show the following options for bridged messages on mobile:
We should not show: Delete for everyone |
thank you very much @pavloburykh for checking. Will update the PR as per summary |
108b920
to
4f8e837
Compare
thank you! I have updated summary and added 'Delete for me` based on our conversation in Discord. |
@flexsurfer, as we have confirmed that bridge messages should behave somewhat like regular messages, please can we go ahead with PR? |
d27f315
to
7a5b8f5
Compare
Thank you @churik for finding this bug, should be fixed now. |
No more issues from my side, thank you for your amazing work!!! |
88% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (46)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
@churik @Parveshdhull, E2E failures seem to be not related to the PR. So it is ready for merge. |
Hi @pavloburykh, @churik, Thank you very much for testing the PR. I just realized that I forgot to add the status-go label to the PR. It's a very small 2-line change, but as soon as I will rebase it will pull more changes. So as we are close to the release branch cutting,
cc @cammellos |
Hey @Parveshdhull! We will need to re-run e2e tests after go branch is rebased in this PR in order to make sure develop go commits will not brake mobile develop after bumping go version. |
7a5b8f5
to
5a47eb3
Compare
87% of end-end tests have passed
Failed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (45)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
|
@pavloburykh Please can you check e2e test results. |
5a47eb3
to
ecfe449
Compare
Thank you @Parveshdhull! E2E failures are not PR related. Ready for merge! |
81% of end-end tests have passed
Failed tests (7)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (42)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
|
fixes #19977
fixes #19976
status-go PR: status-im/status-go#5192
Testing Note:
Only newly fetched messages will be parsed.
status: ready