feat: support edge case flow in the wallet send flow when token is not available on receiver preferred networks #19674
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
Jenkins BuildsClick to see older builds (97)
|
briansztamfater
force-pushed
the
feat/token-network-not-supported-flow
branch
3 times, most recently
from
724716f
to
c449eba
Compare
|
|
briansztamfater
force-pushed
the
feat/token-network-not-supported-flow
branch
5 times, most recently
from
90d6c6c
to
44111df
Compare
briansztamfater
changed the title
briansztamfater
force-pushed
the
feat/token-network-not-supported-flow
branch
from
3027db9
to
ea2796b
Compare
briansztamfater
requested review from
vkjr,
J-Son89,
OmarBasem,
ulisesmac,
mmilad75 and
BalogunofAfrica
J-Son89
reviewed
May 10, 2024
src/quo/foundations/colors.cljs
Outdated
| @@ -188,6 +188,8 @@ | |||
| (def danger "#E95460") | |||
|
|
|||
| ;; Danger with transparency | |||
| (def danger-opa-5 (alpha danger 0.05)) | |||
There was a problem hiding this comment.
we don't need these colors btw,
instead it's better to use
(colors/resolve-color :danger theme 5)
(colors/resolve-color :danger theme 10)
J-Son89
reviewed
May 10, 2024
| @@ -229,13 +229,13 @@ | |||
|
|
|||
| (defn make-network-item | |||
| "This function generates props for quo/category component item" | |||
| [{:keys [network-name color on-change networks state label-props]}] | |||
| [{:keys [network-name color on-change networks state label-props normal-checkbox?]}] | |||
There was a problem hiding this comment.
or more so, what is a not normal checkbox? 🤔
There was a problem hiding this comment.
The idea was to always use the same type, so added a type prop with a fallback value.
J-Son89
reviewed
May 10, 2024
| @@ -20,3 +20,21 @@ | |||
| constants/arbitrum-mainnet-chain-id)) | |||
| (is (= (utils/network->chain-id {:network :arbitrum :testnet-enabled? true :goerli-enabled? false}) | |||
| constants/arbitrum-sepolia-chain-id)))) | |||
|
|
|||
| (deftest test-network-ids->formatted-text | |||
J-Son89
reviewed
May 10, 2024
| :disabled-chain-ids disabled-from-chain-ids | ||
| :receiver-networks receiver-networks | ||
| :token-networks-ids token-networks-ids | ||
| :to? false}) |
There was a problem hiding this comment.
as mentioned by @OmarBasem in another pr to? is a bit confusing.
Perhaps this can be communicated better. Even receiver? or sender? is a bit clearer imo
There was a problem hiding this comment.
Yep, I'll change it in this PR
There was a problem hiding this comment.
Updated, check #19674 (comment)
J-Son89
reviewed
May 10, 2024
| (def token-not-available-text | ||
| {:height 36 | ||
| :flex 1 | ||
| :color colors/danger-50}) |
There was a problem hiding this comment.
please update this to use (colors/resolve-color :danger theme 50). Imo it's better that we abstract the colors resolving to a system so we can ensure it follows the same mechanism in most cases and allows for easier future updates.
There was a problem hiding this comment.
Sure, will update!
There was a problem hiding this comment.
Hmmm actually we need to use danger-50 color which is a solid variant of danger family colors, resolve-color would just apply opacity to danger color.
There was a problem hiding this comment.
Whoops leave out the opacity var and this is correct 👍
There was a problem hiding this comment.
ie (colors/resolve-color :danger theme)
There was a problem hiding this comment.
Now updated it to use (colors/resolve-color :danger theme) 👍
J-Son89
reviewed
May 10, 2024
| {:token-symbol token-symbol | ||
| :network-values sender-network-values | ||
| :on-press #(disable-chain | ||
| %1 |
There was a problem hiding this comment.
maybe it's clearer to use fn here rather than # and explain the var use?
J-Son89
reviewed
May 10, 2024
| #(or (and to? (contains? receiver-networks-set (:chain-id %))) | ||
| (and (not to?) | ||
| (not (contains? disabled-chain-ids (:chain-id %)))))) | ||
| (fn [%] |
There was a problem hiding this comment.
can you name this var? I guess it's network? 🤔
J-Son89
reviewed
May 10, 2024
| @@ -11,12 +12,14 @@ | |||
| [utils.re-frame :as rf])) | |||
|
|
|||
| (defn view | |||
| [{:keys [selected-networks account watch-only?]}] | |||
| [{:keys [title first-section-label second-section-label selected-networks | |||
There was a problem hiding this comment.
should some of these props be in the next fn[] instead? 🤔 I guess the page won't receive the updates if these update otherwise.
There was a problem hiding this comment.
Moved warning-label, the rest should be pretty static props
briansztamfater
force-pushed
the
feat/token-network-not-supported-flow
branch
from
570bf65
to
7732f9d
Compare
|
@VolodLytvynenko thanks for the feedback, issues should be fixed now 🙏 |
85% of end-end tests have passedFailed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
|
|
hi @briansztamfater thank you for PR. No issues from my side. Ready to be merged |
VolodLytvynenko
added
test cases added
and removed
Tested - Issues
test cases required
labels
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 #19670
Summary
This PR adds support for an edge case in the send flow when selected token is not available in the receiver preferred networks. Check the issue and Figma for more details and info about the approach to this edge case.
As part of this issue, I modified network preferences bottom sheet view to support both receiver and sender network settings and avoid duplicate code. So I would suggest to QA also network preferences bottom sheet in other parts of the app where it is used to be sure I didn't broke anything 🙏
Demo 1
sendtounsupportedchain.mp4
Demo 2
sendtounpreferrednetworkdemo2.mp4
Platforms
Areas that maybe impacted
Functional
Steps to test
Case 1
opt:prefix. For exampleopt:0x4328490234823094839043092489394893948934Not availabletext https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=9274-75500&mode=design&t=totQsXRa8D6Rpf2Y-4Case 2
opt:prefix. For exampleopt:0x4328490234823094839043092489394893948934Not availabletext https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=9274-75500&mode=design&t=totQsXRa8D6Rpf2Y-4Add networks SNT can be sent toto automatically add SNT available networksstatus: ready