New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: recalculate network max value #19953
Conversation
Jenkins BuildsClick to see older builds (57)
|
{token-symbol :symbol | ||
token-networks :networks} (rf/sub [:wallet/wallet-send-token]) | ||
enabled-from-networks (->> token-networks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabled from networks seems like it should be a sub with this data instead? 🤔
(filter #(not (contains? (set disabled-from-chain-ids) | ||
(:chain-id %)))) | ||
set) | ||
enabled-from-chain-ids (map :chain-id enabled-from-networks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact if enabled-from-chain-ids
is all you need than all of this computation should be done in the sub:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, its not just the enabled-from-chain-ids
, I am also passing the enabled-from-networks
to filter the chain icons that is displayed
@@ -249,11 +249,13 @@ | |||
:wallet/current-viewing-account-tokens-filtered | |||
:<- [:wallet/current-viewing-account] | |||
:<- [:wallet/network-details] | |||
(fn [[account networks] [_ query]] | |||
(fn [[account networks] [_ query chain-ids]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm is there not tests on this sub already?
If so we should probably add some coverage of this approach to test this behaviour too 👍
ca7ce32
to
01eaf38
Compare
Gonna test your pr shortly @BalogunofAfrica 🔍 |
also not directly related to your pr but just the general UX on the send screen is very unclear to me. Not sure what others think on this but it is a bit confusing of what the expected behaviour is |
Not sure about the exact steps to reproduce this (taps are not shown on the video and it is confusing), but if a chain is disabled, the network card should always be visible with value 0 and opacity, so if that is not happening it is a bug and I can look at it. If that is the case, feel free to file an issue with some steps to reproduce it and assign it to me. |
e2578af
to
9ef9abb
Compare
d304283
to
04ecc49
Compare
@BalogunofAfrica I see you updated the PR to also cover #19910. Can you please update the description and steps to test for manual QA? Also a quick video with the updated changes would be useful 🙏 |
65% of end-end tests have passed
Failed tests (16)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (34)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
04ecc49
to
c907f48
Compare
Done |
047eec5
to
4f0cad8
Compare
e2e failures are not PR related |
4f0cad8
to
7a53303
Compare
7a53303
to
4949aa9
Compare
Hi @BalogunofAfrica thank you for your work. No issues from my side. PR is ready to be merged |
30d452c
to
ac19c87
Compare
ac19c87
to
d1968f6
Compare
@VolodLytvynenko seems this PR introduced other bugs and I think it doesn't fully fix #19910 See this, we always display "No routes found" when user enters an invalid amount: Can you please verify? Maybe we should revert this PR, or introduce proper fixes in another PR. |
Fixes
#19791
#19910
Summary
Recalculates the max value based on the chain the user select in the from column
Platforms
Functional
Steps to test
status: ready