[#20479] - Skip "From" page on wallet home #20773
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
ulisesmac
requested review from
vkjr,
briansztamfater,
J-Son89,
mmilad75 and
a team
ulisesmac
commented
Jul 16, 2024
Comment on lines
+19
to
+26
| [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.
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.
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.
@briansztamfater Sure!
It's a great suggestion, I'll apply it!
There was a problem hiding this comment.
@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.
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)
|
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
ulisesmac
commented
Jul 16, 2024
J-Son89
reviewed
Jul 18, 2024
43% of end-end tests have passedFailed 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. |
ulisesmac
force-pushed
the
20479-skip-from-page-and-remove-account-switcher
branch
from
3e31a30
to
5605c00
Compare
|
@J-Son89 @briansztamfater |
ulisesmac
force-pushed
the
20479-skip-from-page-and-remove-account-switcher
branch
from
769b4b7
to
f915385
Compare
71% of end-end tests have passedFailed 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. |
briansztamfater
approved these changes
Jul 22, 2024
71% of end-end tests have passedFailed 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 |
J-Son89
approved these changes
Jul 23, 2024
…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
ulisesmac
force-pushed
the
20479-skip-from-page-and-remove-account-switcher
branch
from
f915385
to
407b486
Compare
This was referenced Jul 25, 2024
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 #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