-
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
[#20479] - Skip "From" page on wallet home #20773
[#20479] - Skip "From" page on wallet home #20773
Conversation
[send-params entry-point] | ||
{:icon :i/send | ||
:accessibility-label :send | ||
:label (i18n/label :t/send) | ||
:on-press (fn [] | ||
(rf/dispatch [:hide-bottom-sheet]) | ||
(rf/dispatch [:wallet/clean-send-data]) | ||
(rf/dispatch [:wallet/set-token-to-send send-params]))}) | ||
(rf/dispatch [:wallet/set-token-to-send send-params entry-point]))}) |
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.
An entry-point
parameter has been added, this will define where we are pressing the token and what information we should add or remove while navigating forward and backward respectively.
Currently only exists :wallet-home
(added when we send from the wallet home)
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.
Could we use the current screen name as the key instead of using arbitrary keys like :wallet-home
? If not, can we create constants for these keys to make the code more maintainable? This way, we'll have a single source of truth for the possible values of the entry-point
parameter.
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.
@briansztamfater Sure!
It's a great suggestion, I'll apply it!
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.
@briansztamfater The wallet home component is not mounted on the screen navigation hierarchy, so that it doesn't have a screen name. Creating the keyword :screen/wallet-home
for this purpose is misleading, so better to keep it as is, but for other valid entry-points I'll apply your suggestion.
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.
AFAIK, you can get the screen name from :view-id
key in app-db. From what I could find by debugging with re-frisk, "wallet home" screen name is :wallet-stack
(as it is the root screen of wallet tab stack).
Jenkins BuildsClick to see older builds (15)
|
43% of end-end tests have passed
Failed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (3)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
hi @ulisesmac Thank you for fixes. Apologies, I was not so familiar with the flow when I reported the original issue. We have some more places from which we can be navigated to the "FROM" page. Would it be better to fix this separately or in the current PR? ISSUE 1: The FROM page is shown when user goes to send flow using scannerPrecondition:User has only one account Steps:
Actual result:User sees the 1 account in from page sendflow.mp4Expected result:User is navigated directly to the send flow page if user has only 1 account and use scanner during send flow |
ISSUE 2: From is shown when user goes to sending flow via bridgePreconditions:User has Account 1 Steps:
Actual result:User sees the FROM page bridge.mp4Expected result:when "bridge" button is long-tapped from Wallet Home page and the selected token to be sent is available on at least two accounts:
|
@VolodLytvynenko Thanks for reporting these entry points. I'll fix them in this PR. UPDATE:Issue 1I think this issue is completely different, it's not related to skip the "From" page when only one user owns an asset, it could be written as: "Skip From page when there's only one account in the device", so this should be addressed separately and the issue should already have the answers for the following quesitons: If there's no assets and there's only one account, should we completely remove the "send" flow from everywhere (not only the "From" page)? How would they look? what will we do instead? Do we need need new designs/states for these cases? Issue 2The bridge flow is a completely different flow compared to send, it requires a deeper understanding of the data it handles and its flows/entry-points, so it'd signifincantly require more time to get solved and this PR will grow a lot in terms of code. Better to solve it separately. What I did that works well is omiting the Bridge option when there is zero accounts owning an asset: Screencast.from.2024-07-19.15-44-41.mp4The problem is not just removing the From page, it's about making sure the other flows still show the option and nothing is broken, so, again, better to solve it separately. |
3e31a30
to
5605c00
Compare
@J-Son89 @briansztamfater |
769b4b7
to
f915385
Compare
71% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
hi @ulisesmac Yes, you are right. The expected result should be "Skip the From page when there's only one account on the device." I discussed this with @xAlisher and described it in the expected behavior of issue 1. Requirement 1:
Requirement 1 (alternative implementation):If Requirement 1 is too complicated to implement: For both sending via the scanner and when an asset is long-pressed and the "send" or "bridge" button is tapped, if more than one account exists (excluding watch-only accounts), show the FROM page. Otherwise, skip it. |
71% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
|
hi @ulisesmac thank you for PR. no issues from my side. PR is ready to be merged |
…account - Update tests - Omit from page while sending a token in home page - Hide send and bridge option for not owned tokens - Fix subscription to return accounts owning an asset
f915385
to
407b486
Compare
fixes #20479
Summary
This PR is not addressing
yet, but it'll be addressed in a follow-up.
The "From" page now it's not shown when we only own one account with the desired asset to send.
Demo 1 (ETH):
Screencast.from.2024-07-16.16-18-32.mp4
Demo 2 (USDC):
Screencast.from.2024-07-16.16-20-48.mp4
Demo 3 (Collectible):
Screencast.from.2024-07-16.16-24-36.mp4
Review notes
Right now, a lot of flows depend on the current state of the re-frame DB, so, in order to not break anything and keep things flexible, some changes have been made in the APIs. These changes are made in a flexible way (by extending the APIs instead of adding constraints to them).
Testing notes
Please test the existing send flows are still working, I've tested them but, since the mechanism is becoming more and more complex, there might be a forgotten edge case.
Steps to test
status: ready