chore(wallet): add flow for selecting own accounts in send flows #18071
Conversation
|
|
||
| (def view (quo.theme/with-theme view-internal)) |
There was a problem hiding this comment.
we only need with-theme HOC if we need direct access to the value of "theme"
| input-value (reagent/atom "") | ||
| input-focused? (reagent/atom false)] | ||
| (fn [] | ||
| (let [valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?]))] | ||
| (let [selected-tab (or (rf/sub [:wallet/send-tab]) (:id (first tabs/data))) |
There was a problem hiding this comment.
changed to using rf here as we need to persist the selected tab for navigating back etc
There was a problem hiding this comment.
Nice, just make sure that when we navigate back and forth from this screen, the re-frame entry is reset, so the focused tab is the first one.
There was a problem hiding this comment.
is that what we want? I would have thought we want to be on the same tab we left from? 🤔
There was a problem hiding this comment.
🤔 when you go back I think it's expected to keep the state, but when you go forward I think the user expects to start from a "new" state.
But it might be just a personal perspective 🤔
J-Son89
requested review from
briansztamfater,
ulisesmac,
smohamedjavid,
tumanov-alex,
OmarBasem and
mmilad75
Jenkins BuildsClick to see older builds (31)
|
| (rf/reg-sub | ||
| :wallet/other-accounts | ||
| :<- [:wallet/accounts] | ||
| :<- [:wallet/current-viewing-account-address] | ||
| (fn [[accounts current-address]] | ||
| (filter #(not= (:address %) current-address) accounts))) | ||
|
|
There was a problem hiding this comment.
There is an existing sub :wallet/accounts-without-current-viewing-account which gives out the same result.
There was a problem hiding this comment.
Thanks I'll use that instead
| [] | ||
| (let [margin-top (safe-area/get-top) | ||
| selected-tab (reagent/atom (:id (first tabs-data))) | ||
|
|
J-Son89
force-pushed
the
jc/my-accounts
branch
from
f2b4e8b to
4630277
Compare
| (ns status-im2.contexts.wallet.send.events | ||
| (:require | ||
| [utils.number] | ||
| [utils.re-frame :as rf])) |
There was a problem hiding this comment.
Nice you created a new namespace 👍
btw, I think utils.number is unused
| input-value (reagent/atom "") | ||
| input-focused? (reagent/atom false)] | ||
| (fn [] | ||
| (let [valid-ens-or-address? (boolean (rf/sub [:wallet/valid-ens-or-address?]))] | ||
| (let [selected-tab (or (rf/sub [:wallet/send-tab]) (:id (first tabs/data))) |
There was a problem hiding this comment.
Nice, just make sure that when we navigate back and forth from this screen, the re-frame entry is reset, so the focused tab is the first one.
| (rf/reg-sub | ||
| :wallet/send-tab | ||
| :<- [:wallet/ui] | ||
| (fn [ui] | ||
| (get-in ui [:send :select-address-tab]))) |
| (rf/dispatch [:navigate-to-within-stack | ||
| [:wallet-select-asset :wallet-select-address]]))}]) | ||
|
|
||
| (def data |
There was a problem hiding this comment.
I think this can have a better name than data. Maybe tabs or tabs-data.
Also I think it is not used in the current file?
There was a problem hiding this comment.
it's used in the main view, I can rename it 👌
There was a problem hiding this comment.
moved it to main view 👍
J-Son89
force-pushed
the
jc/my-accounts
branch
from
4630277 to
bfb05ba
Compare
J-Son89
force-pushed
the
jc/my-accounts
branch
from
bfb05ba to
74cb6c8
Compare
79% of end-end tests have passedNot executed tests (1)Failed tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (38)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
100% of end-end tests have passedPassed tests (1)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
|
|
Hi @J-Son89 Could you please rebase the PR and resolve existing conflicts? |
J-Son89
force-pushed
the
jc/my-accounts
branch
from
74cb6c8 to
47ef4e6
Compare
|
done @VolodLytvynenko - but give me a few mins to verify the build is all good etc and I'll ping you to let you know once I do a quick smoke test 👍 |
J-Son89
force-pushed
the
jc/my-accounts
branch
from
47ef4e6 to
8b45c72
Compare
86% of end-end tests have passedExpected to fail tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
Hey. @J-Son89 thank you for the PR. I didn't encounter any issues, just a few questions that might be considered as potential issues: Question 1: Should the watch-only page be selectable on the "send to" page?Description: From my understanding, the 'send to' page is restricted to non-editable, watch-only accounts. However, a user can access it by selecting it using the select account icon (shown in the right upper corner)Steps:
Actual result:The user lands on the 'send to' page of the watch-only account and can view accounts from the 'my account' tab to send assets. watchonly.mp4Expected result:Potential solution: |
Question 2: Which accounts should display the 'wallet' icon? Currently, it is not visible for any account type.Actual result:The wallet icon is not shown for specific accounts on the "sent to" page Expected result:The wallet icon is shown for specific accounts on the "sent to" page |
|
@J-Son89 If the questions above aren't related to the current PR and don't require new commits to address them, then the PR can be merged. However, please remember to move it to the design review if this PR needs design feedback |
Thanks @VolodLytvynenko, nice spot! - Question 1 is definitely a bug because we can't send anything from Watch-Only accounts? If it's okay I will handle this in a separate issue/pr as it is beyond the scope of this work and not introduced here. |
@J-Son89 Sure. I will create it separately. Thanx |
J-Son89
force-pushed
the
jc/my-accounts
branch
from
8b45c72 to
8c0c802
Compare
J-Son89
force-pushed
the
jc/my-accounts
branch
2 times, most recently
from
8596113 to
a01a5d5
Compare
|
@VolodLytvynenko - after checking we will add support for Keycard soon, as such I will leave adding that icon until a future issue as it involves having keycard properly setup on mobile first to verify this. |
J-Son89
force-pushed
the
jc/my-accounts
branch
from
a01a5d5 to
1ad6fc1
Compare
@J-Son89 Yeh. Thank you for the clarification and for managing all of these wallet follow ups |
fixes: #16892
Figma Reference :
https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=689-192739&mode=design&t=EXPLGpRJgQikZol4-4
This pr adds the data for the flow when the user is selecting the account to send to. Navigate to the new wallet, select an account, hit the "send" button. Go to my accounts tab ->
trim.AEB84561-76FB-46E4-B42D-D4930268D1E0.MOV