Support for multiple collectibles transaction on confirmation page #19502
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
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
780ef08
to
cc215ae
Compare
Jenkins BuildsClick to see older builds (16)
|
J-Son89
reviewed
Apr 4, 2024
| [chain-id] | ||
| (let [network-name (-> (rf/sub [:wallet/network-details-by-chain-id chain-id]) | ||
| :network-name)] | ||
| (if (= network-name :mainnet) :ethereum network-name))) |
There was a problem hiding this comment.
@OmarBasem, @smohamedjavid - don't we already have a solution for this somewhere else? 🤔
There was a problem hiding this comment.
I think Javid is handling it in one of his PRs which is not merged yet
J-Son89
reviewed
Apr 4, 2024
| (if (= network-name :mainnet) :ethereum network-name))) | ||
|
|
||
| (defn network-amounts-from-route | ||
| [route token-symbol token-decimals to?] |
There was a problem hiding this comment.
can we make this a map? 🙏
Also it can be private?
J-Son89
reviewed
Apr 4, 2024
| {} | ||
| route)) | ||
|
|
||
| (defn network-values-from-amounts |
J-Son89
reviewed
Apr 4, 2024
| (defn network-values-from-amounts | ||
| [network-amounts token-symbol] | ||
| (reduce-kv (fn [acc k v] | ||
| (assoc acc k {:amount v :token-symbol token-symbol})) |
There was a problem hiding this comment.
maybe align the keys for readability?
J-Son89
reviewed
Apr 4, 2024
| {} | ||
| network-amounts)) | ||
|
|
||
| (defn sanitize-network-values |
There was a problem hiding this comment.
we already have this function, no?
cc @briansztamfater
J-Son89
reviewed
Apr 4, 2024
| [{:keys [account-props theme label accessibility-label | ||
| summary-type values] | ||
| :as props}] | ||
| (tap> props) |
J-Son89
reviewed
Apr 4, 2024
| :token-symbol token-symbol | ||
| :route route | ||
| :token-decimals token-decimals | ||
| :to? true})}] |
There was a problem hiding this comment.
this file looks much cleaner with how you have organised it!
Nice one @vkjr! 🚀
|
@status-im/wallet-mobile-devs - can someone give this pr a quick happy path check? |
We moved and refactored most of the logic for calculating amounts of each network to the send events in #19392, so I think we should wait. Though this PR looks good, I think transaction confirmation view it will be simplified and many of the functions modified here will be removed in favor of events and subscriptions. |
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
3f1ee4d
to
f51725b
Compare
|
@J-Son89 @briansztamfater , |
Cool, whatever you and @briansztamfater think. All good by me either way 👍 |
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
f51725b
to
bd82e29
Compare
0% of end-end tests have passedFailed tests (51)Click to expandClass TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
92% of end-end tests have passedFailed tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (48)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
|
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
bd82e29
to
bcddfd7
Compare
|
e2e failures are not related to PR. |
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
bcddfd7
to
1db8eb2
Compare
vkjr
force-pushed
the
multiple-collectibles-transaction
branch
from
1db8eb2
to
5c4b06b
Compare
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 #19142
Summary
Screen.Recording.2024-04-03.at.16.12.59.mov
Figma designs
Review notes
Functional changes are minimal. Added a few conditionals to correctly display collectible image in title and name of the collectible in
fromandtosection.Everything else is a refactoring for better readability.
status: ready