Fixes for screen where user locks network amounts to send #20519
+70
−39
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 (8)
|
J-Son89
reviewed
Jun 21, 2024
| @@ -366,7 +366,7 @@ | |||
| :on-token-press show-select-asset-sheet}] | |||
| [routes/view | |||
| {:token token-by-symbol | |||
| :input-value input-amount | |||
| :send-amount-in-crypto amount-in-crypto | |||
J-Son89
reviewed
Jun 21, 2024
| send-amount-in-crypto-bn (money/bignumber send-amount-in-crypto) | ||
| lock-higher-than-send-amount? (if (and (money/bignumber? amount-in-crypto-bn) | ||
| (money/bignumber? | ||
| send-amount-in-crypto-bn)) |
There was a problem hiding this comment.
I find -bn to be a bit more confusing. Can we name it something else? perhaps we can just pass it as that? 🤔
J-Son89
reviewed
Jun 21, 2024
| @@ -186,6 +198,12 @@ | |||
| :show-keyboard? false | |||
| :value (controlled-input/input-value input-state) | |||
| :on-swap swap-between-fiat-and-crypto}] | |||
| (when lock-higher-than-send-amount? | |||
There was a problem hiding this comment.
what is lock-higher than. should be like locked-greater-then-send-amount? ?
J-Son89
reviewed
Jun 21, 2024
| :disabled | ||
| :else type)] | ||
| [rn/view | ||
| {:key (str (if receiver? "to" "from") "-" chain-id) |
There was a problem hiding this comment.
"to and "from" should be i18n labels, no? perhaps they both should take this chain-id as a param too so they're translatable?
There was a problem hiding this comment.
Well, it is :key, so it is probably unimportant. Moreover, we aren't sure how react will work with translated keys)
J-Son89
reviewed
Jun 21, 2024
translations/en.json
Outdated
| @@ -2709,5 +2709,6 @@ | |||
| "not-enough-assets": "Not enough assets to pay gas fees", | |||
| "send-from-network" : "Send from {{network}}", | |||
| "define-amount-sent-from-network" : "Define amount sent from {{network}} network", | |||
| "dont-auto-recalculate-network": "Don't auto recalculate {{network}}" | |||
| "dont-auto-recalculate-network": "Don't auto recalculate {{network}}", | |||
| "value-higher-than-send-amount": "This value is higher than entered send amount" | |||
There was a problem hiding this comment.
this text looks a bit off, perhaps we revisit? 🤔
i.e "This value is higher than entered send amount"
should be like?
"This value is higher than entered amount to send" ??
J-Son89
approved these changes
Jun 21, 2024
vkjr
force-pushed
the
fixes-for-amount-locking
branch
from
8d0a220
to
1246e93
Compare
ulisesmac
approved these changes
Jun 24, 2024
Comment on lines
140
to
147
| locked-greater-then-send-amount? (let [amount (money/bignumber | ||
| amount-in-crypto) | ||
| send-amount (money/bignumber | ||
| send-amount-in-crypto)] | ||
| (if (and (money/bignumber? amount) | ||
| (money/bignumber? send-amount)) | ||
| (money/greater-than amount send-amount) | ||
| false)) |
There was a problem hiding this comment.
I think this if can be simplified to just use the and:
(and (money/bignumber? amount)
(money/bignumber? send-amount)
(money/greater-than amount send-amount))There was a problem hiding this comment.
Great suggestion, thanks!
Comment on lines
246
to
247
| (map-indexed (fn [index {:keys [chain-id total-amount type]}] | ||
| [rn/view | ||
| {:key (str (if receiver? "to" "from") "-" chain-id) | ||
| :style {:margin-top (if (pos? index) 11 7.5)}} | ||
| [quo/network-bridge | ||
| {:amount (if (= type :not-available) | ||
| (i18n/label :t/not-available) | ||
| (str total-amount " " token-symbol)) | ||
| :network (network-utils/id->network chain-id) | ||
| :status (cond (and (= type :not-available) | ||
| loading-routes? | ||
| token-not-supported-in-receiver-networks?) | ||
| :loading | ||
| (= type :not-available) | ||
| :disabled | ||
| :else type) | ||
| :on-press #(when (not loading-routes?) | ||
| (cond | ||
| (= type :edit) | ||
| (open-preferences) | ||
| on-press (on-press chain-id total-amount))) | ||
| :on-long-press #(when (not loading-routes?) | ||
| (cond | ||
| (= type :add) | ||
| (open-preferences) | ||
| on-long-press (on-long-press chain-id)))}]]) | ||
| (let [status (cond (and (= type :not-available) |
There was a problem hiding this comment.
Better to destructure type using other name, I've seen many errors due to override clojure.core fns, such as name, key or type
80% of end-end tests have passedFailed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (41)Click to expandClass TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
|
vkjr
force-pushed
the
fixes-for-amount-locking
branch
from
1246e93
to
4e15db0
Compare
|
@vkjr Thank you for PR. Just a few issues are found ISSUE 1: Checkbox not checked after value removal in assets customization drawerSteps:
Actual result:The checkbox is still shown as unchecked checkbox.mp4Expected result:The checkbox should be checked after any updates are made in the drawer, including the removal of the value |
PR_ISSUE 2: Predefined value not refreshed in drawer after asset changeSteps:
Actual result:The previous value is predefined change_asset.mp4Expected result:The previous value should be refreshed after the asset is changed. OS:IOS, Android Devices:
|
|
this issue is also present in develop. Could you fix this in the scope of the current PR? If not, I can create it separately ISSUE 3: Wrong UI of checkbox after uncheckingSteps:Unfortunately, there are no exact steps to reproduce this issue, but it happens very often. Just try to check and uncheck the checkbox a few times to reproduce it successfully. Actual result:The UI of the checkbox is wrong after unchecking. checbox2.mp4Expected result:The UI of the checkbox should not be broken after unchecking. Devices:
|
|
@VolodLytvynenko, thanks for the findings! |
vkjr
force-pushed
the
fixes-for-amount-locking
branch
from
4e15db0
to
fb39d6a
Compare
|
@VolodLytvynenko, all issues should be resolved now) |
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 #20497
fixes #20496
fixes #20498
Summary
Screen.Recording.2024-06-20.at.19.32.48.mov
Areas that maybe impacted
Functional
status: ready