-
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
Enable user to share wallet address #18424
Conversation
@@ -0,0 +1,75 @@ | |||
(ns status-im.contexts.shell.share.profile.view |
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.
Created a new file share/profile/view
and moved the profile
code below, that was previously in share/view
, to improve code organization and readability.
@@ -1,18 +1,14 @@ | |||
(ns status-im.contexts.shell.share.view | |||
(:require | |||
[clojure.string :as string] |
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.
The removed code in this file was moved to the new file above - share/profile/view
If we do it on top of the current window (like it is currently implemented in this PR) user stays in the selected app and is not even redirected back to Status share screen after sending wallet address. That does not look user friendly IMO. Because in this case user needs to tap system back button couple of times until he is returned to initial Share wallet screen. Also It looks weird that after sharing wallet address User can continue interacting with selected app that is opened on top of Status app window. |
@OmarBasem, is this a common issue to the wallet share screens? If so perhaps it is beyond the scope of this pr? 🤔 |
@J-Son89 if by "wallet share screen" you mean this screen below - then the answer is yes. ISSUE 1 and ISSUE 3 are common. So probably those issues can be handled separately, not in scope of this PR. |
Thanks @pavloburykh - yes exactly, I mean those screens. Ok perfect, sounds good 👌 |
Okay, so do I understand correctly that among all above issues we want to fix only ISSUE 2 in scope of this PR? Or ISSUE 2 is also considered not to be in scope? @OmarBasem @J-Son89 |
Oh issue 2 looks related to this pr 👌 |
@J-Son89 Issue 1 has been fixed and tested by @pavloburykh |
@J-Son89 Issue 3 has also been fixed and tested by @pavloburykh |
77% of end-end tests have passed
Failed tests (9)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (37)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
|
Thank you @Pau1fitz So, here the summury: ISSUE 1 and 3 are fixed. ISSUE 2 is fixed partially: still can scroll QR up and down. We agreed to log this issue separately as it is device/IOS specific. telegram-cloud-document-2-5260607374435044659.mp4PR is ready for merge. |
@Pau1fitz - I think you have to sign commits for the pr to be merged https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits |
@J-Son89 I spoke to @flexsurfer about this and I am happy for a core dev to merge this on my behalf. |
merged here: #18511 |
Fixes #17865
Share Screen -> Implement Share Wallet Address
Screen.Recording.2024-01-06.at.21.29.07.mov
Figma design
iOS
Android
Testing notes:
Navigate to the share screen
click on Wallet
user can see each wallet that to share address