fix: routes ui for bridge flow #19959
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 (25)
|
briansztamfater
force-pushed
the
fix/bridge-routes-ui
branch
2 times, most recently
from
e141474
to
4dcf3e6
Compare
briansztamfater
changed the title
briansztamfater
requested review from
vkjr,
smohamedjavid,
J-Son89,
OmarBasem,
ulisesmac and
mmilad75
OmarBasem
reviewed
May 10, 2024
Comment on lines
294
to
295
| token-available-networks-for-suggested-routes) | ||
| token-available-networks-for-suggested-routes) |
There was a problem hiding this comment.
I am not sure what does token-available-networks-for-suggested-routes mean, is it the available routes for the user's network preferences?
There was a problem hiding this comment.
Are the possible networks where a route could be found for the selected token.
| @@ -56,13 +57,15 @@ | |||
| (send-utils/network-amounts from-network-values-for-ui | |||
| disabled-from-chain-ids | |||
| receiver-networks | |||
| transaction-type | |||
There was a problem hiding this comment.
I see sometimes transaction-type and others tx-type
There was a problem hiding this comment.
I'll refactor to just use tx-type 👍
Comment on lines
147
to
152
| (filter | ||
| #(or (and to? | ||
| (or | ||
| (= tx-type :bridge) | ||
| (contains? receiver-networks-set (:chain-id %)))) | ||
| (not to?))) |
There was a problem hiding this comment.
The logic differs a bit if we are calculating networks for the From column or the To column, so it is a flag that indicates whether if we are calculating things for receiver or sender.
There was a problem hiding this comment.
Refactored this on other PR. Check #19674 (comment)
There was a problem hiding this comment.
I think you can refactor the relevant changes here too, or they may overwrite the other changes (?)
There was a problem hiding this comment.
They will conflict when I merge one of both PRs so I prefer to so refactor them once and rebase
|
PR description missing issue number, is there an open issue to be linked? |
@OmarBasem I haven't found one, I found the bug while testing and started to work on it, I will create one issue edit the description. EDIT: Created #19970 and linked to it in the description |
briansztamfater
force-pushed
the
fix/bridge-routes-ui
branch
from
4dcf3e6
to
e9e06b9
Compare
OmarBasem
approved these changes
May 13, 2024
88% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
J-Son89
approved these changes
May 14, 2024
qoqobolo
force-pushed
the
fix/bridge-routes-ui
branch
from
533b755
to
d95def4
Compare
|
Testing of wallet PRs is blocked until the problem with networks/balances is resolved #20011 |
|
BTW, I need to solve conflicts before testing |
Hey @briansztamfater, tbh it is not completely clear what and where exactly the problem is: those banners with It might also be better to test and update go version which includes changes to the router first and then test this PR, WDYT? |
briansztamfater
force-pushed
the
fix/bridge-routes-ui
branch
3 times, most recently
from
2cdd919
to
214e7aa
Compare
|
No problem @qoqobolo, I finished with the rebase and solving conflicts, so now is ready to test whenever you feel the conditions are met 👍 . |
87% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
|
|
Thanks for your work and patience @briansztamfater! |
briansztamfater
force-pushed
the
fix/bridge-routes-ui
branch
from
214e7aa
to
725a57a
Compare
Signed-off-by: Brian Sztamfater <brian@status.im>
briansztamfater
force-pushed
the
fix/bridge-routes-ui
branch
from
725a57a
to
07e81de
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 #19970
Summary
This PR fixes broken UI for routes on bridge flow (+ button should not be displayed in
Tocolumn, also selected network is not showing). Also added new unit tests on relevant utils functions to cover these cases.Testing notes
Bridge flow seems broken and user can't confirm Bridge operation, this PR only fixes routes ui.
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
Before
ridgeroutesbefore.mp4
After
bridgeroutesui.mp4
status: ready