refactor: suggested routes view #19768
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
Jenkins BuildsClick to see older builds (120)
|
briansztamfater
force-pushed
the
refactor/routes-view
branch
5 times, most recently
from
68b8a8d
to
2304880
Compare
briansztamfater
commented
Apr 24, 2024
Comment on lines
35
to
41
| (defn fetch-routes | ||
| [amount routes-can-be-fetched? bounce-duration-ms] | ||
| (if routes-can-be-fetched? | ||
| (debounce/debounce-and-dispatch | ||
| [:wallet/get-suggested-routes {:amount amount}] | ||
| bounce-duration-ms) | ||
| (rf/dispatch [:wallet/clean-suggested-routes]))) |
There was a problem hiding this comment.
Just moved the function to the top
briansztamfater
force-pushed
the
refactor/routes-view
branch
16 times, most recently
from
bc22785
to
f487eac
Compare
briansztamfater
commented
Apr 25, 2024
Comment on lines
82
to
101
| (fn [{:keys [db]} [error]] | ||
| (let [receiver-network-values (get-in db [:wallet :ui :send :receiver-network-values]) | ||
| sender-network-values (get-in db [:wallet :ui :send :sender-network-values]) | ||
| cleaned-sender-network-values (send-utils/clean-network-amounts sender-network-values) | ||
| cleaned-receiver-network-values (send-utils/clean-network-amounts receiver-network-values)] | ||
| {:db (-> db | ||
| (update-in [:wallet :ui :send] | ||
| dissoc | ||
| :route) | ||
| (assoc-in [:wallet :ui :send :sender-network-values] cleaned-sender-network-values) | ||
| (assoc-in [:wallet :ui :send :receiver-network-values] cleaned-receiver-network-values) | ||
| (assoc-in [:wallet :ui :send :loading-suggested-routes?] false) | ||
| (assoc-in [:wallet :ui :send :suggested-routes] {:best []})) | ||
| :fx [[:dispatch | ||
| [:toasts/upsert | ||
| {:id :send-transaction-error | ||
| :type :negative | ||
| :text (:message error)}]]]}))) |
There was a problem hiding this comment.
We treat endpoint error as if no routes were found so user can retry the request, but we also show a toast to display the error
briansztamfater
commented
Apr 25, 2024
Comment on lines
+228
to
+243
| :on-press (if no-routes-found? | ||
| #(rf/dispatch [:wallet/get-suggested-routes | ||
| {:amount (controlled-input/input-value | ||
| input-state)}]) | ||
| on-confirm)} |
There was a problem hiding this comment.
If no routes where found (because of an error or because the getSuggestedRoutes returned a success response with empty routes), we show the user a Try again button so they can resend the request
There was a problem hiding this comment.
Is this something from the designs? (The Try again button)
There was a problem hiding this comment.
Yes, it is in the "No routes found" design
briansztamfater
force-pushed
the
refactor/routes-view
branch
from
51301ba
to
d7f4a3e
Compare
briansztamfater
requested review from
J-Son89,
OmarBasem,
ulisesmac and
mmilad75
OmarBasem
reviewed
Apr 30, 2024
Comment on lines
262
to
266
| token-id (if token | ||
| (:symbol token) | ||
| (str (get-in collectible [:id :contract-id :address]) | ||
| ":" | ||
| (get-in collectible [:id :token-id]))) |
There was a problem hiding this comment.
This can be a utility:
(defn format-token-id
[token collectible]
(if token
(:symbol token)
(str (get-in collectible [:id :contract-id :address])
":"
(get-in collectible [:id :token-id]))))
Comment on lines
+228
to
+243
| :on-press (if no-routes-found? | ||
| #(rf/dispatch [:wallet/get-suggested-routes | ||
| {:amount (controlled-input/input-value | ||
| input-state)}]) | ||
| on-confirm)} |
There was a problem hiding this comment.
Is this something from the designs? (The Try again button)
83% of end-end tests have passedFailed tests (7)Click to expandClass TestWalletMultipleDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (43)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
briansztamfater
force-pushed
the
refactor/routes-view
branch
from
76cb329
to
857922b
Compare
88% of end-end tests have passedFailed tests (4)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (46)Click to expandClass TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
|
briansztamfater
force-pushed
the
refactor/routes-view
branch
from
857922b
to
9413912
Compare
This was referenced May 2, 2024
|
Hi @briansztamfater, I'm a bit unsure about whether there should be the ability to include several routes, as shown in your screenshots. I hope not, since it's not implemented on the desktop. However, I can't understand how you achieved this on your screens, as shown in the description of the issue. I mean example: My user has more than 0.01 ETH on all networks. When I enter 0.03 into the send field, I expect to see several routes at once:
But instead, I see 'no routes are found'. however, on your screen, I see all three routes. |
|
Hi @VolodLytvynenko thanks for testing! Some clarifications regarding the screenshots in the description: |
|
Hi @briansztamfater, thank you for the PR. I don't have any issues with it. I've asked a few questions regarding the display of network routes. https://discord.com/channels/624634427930312714/852533718790570015/1235591512860131448 https://discord.com/channels/624634427930312714/852533718790570015/1235601862166581278 https://discord.com/channels/624634427930312714/852533718790570015/1235616454351519774 If you think it's best to wait for responses from the design team and make changes based on their answers before merging the PR, please let me know. Otherwise, if you believe it's fine to merge as is, than PR can be merged and I'll track any follow-ups related to the design team's replies separately |
|
Thanks @VolodLytvynenko, I think it is better to merge this because some issues depends on this refactor and we can track any follow up on other issues. If it is okay, I'll merge it as soon as I get into my computer. Thanks again! |
@briansztamfater thank you. You did a great job! |
Signed-off-by: Brian Sztamfater <[email protected]>
briansztamfater
force-pushed
the
refactor/routes-view
branch
from
9413912
to
4f24cb6
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 #19696
Summary
This PR refactors suggested network routes view. Please read the issue for context on the why we needed to refactor the component, but basically it wasn't complying with different cases.
This new approach enables a more flexible UI where different types of network links components are used to connect network summary cards in the suggested routes screen. Some examples:
At the same time it adds some new flows for routes view:
Platforms
Functional
Steps to test
Smoke test:
Disable last chain:
No routes found:
Try againbuttonTry againbutton to search for routes againstatus: ready