wallet - reset input on send flow when swapping accounts #20099
Merged
+272
−266
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
J-Son89
commented
May 19, 2024
| @@ -40,7 +40,6 @@ | |||
| [:catn | |||
| [:props | |||
| [:map {:closed true} | |||
| [:theme :schema.common/theme] | |||
There was a problem hiding this comment.
there was a schema issue as this now uses the context api internally
J-Son89
commented
May 19, 2024
| :label (i18n/label :t/import-private-key) | ||
| :on-press (when (ff/enabled? ::wallet.import-private-key) | ||
| #(rf/dispatch [:navigate-to :screen/wallet.import-private-key]))}]]]) | ||
| (when (ff/enabled? ::wallet.import-private-key) |
There was a problem hiding this comment.
hide this button until it's a complete flow
J-Son89
commented
May 19, 2024
| @@ -91,15 +90,15 @@ | |||
| [rn/view | |||
| [quo/icon :i/alert | |||
| {:size 16 | |||
| :color colors/danger-50}]] | |||
| :color (colors/resolve-color :danger theme)}]] | |||
J-Son89
commented
May 19, 2024
| loading-routes? (rf/sub | ||
| [:wallet/wallet-send-loading-suggested-routes?]) | ||
| bottom (safe-area/get-bottom) | ||
| [crypto-currency? set-crypto-currency] (rn/use-state initial-crypto-currency?) |
There was a problem hiding this comment.
before this page was mixing reagent and react - no bueno :)
J-Son89
commented
May 19, 2024
| (rn/use-effect | ||
| (fn [] | ||
| (set-input-state controlled-input/init-state)) | ||
| [current-address]) |
There was a problem hiding this comment.
This hook flushes the input to it's default state once the current address changes
There was a problem hiding this comment.
checks it is not the default value as otherwise it causes a bug on the initial render. Only want this to fire if the account has changed and the input is not the initial state.
There was a problem hiding this comment.
Hey @J-Son89, what happens if we have routes shown on the screen, and we change the account, are routes cleaned too?
If not, probably we should call (rf/dispatch [:wallet/clean-suggested-routes]) here too.
There was a problem hiding this comment.
Thanks @briansztamfater gonna check this!
There was a problem hiding this comment.
Just noticed this while testing in one of my rebased branches:
If user disables a network and then the account changes, the disabled networks are not reset. I think we should reset disabled networks as well because the switched account might have balance only on the previously disabled networks, leading the user to a dead-end.
For that, we should call (rf/dispatch [:wallet/clean-disabled-from-networks]) here too. We can raise a new PR for this, let me know how would you like to handle it @J-Son89!
There was a problem hiding this comment.
Nice one @briansztamfater ! I can add the pr for this next week 👍
J-Son89
commented
May 19, 2024
| receiver-preferred-networks-set | ||
| receiver-selected-network)) | ||
| receiver-networks)) | ||
| current-address (rf/sub [:wallet/current-viewing-account-address]) |
There was a problem hiding this comment.
current address sub added 👍
Jenkins BuildsClick to see older builds (36)
|
J-Son89
changed the title
J-Son89
commented
May 20, 2024
| @@ -8,8 +8,7 @@ | |||
| (h/render-with-theme-provider component :light)) | |||
|
|
|||
| (def props | |||
| {:theme :light | |||
J-Son89
requested review from
briansztamfater,
OmarBasem,
vkjr,
BalogunofAfrica and
ulisesmac
OmarBasem
approved these changes
May 20, 2024
| (when (not= input-state controlled-input/init-state) | ||
| (set-input-state controlled-input/init-state))) | ||
| [current-address]) | ||
|
|
Comment on lines
261
to
298
| (rn/use-effect | ||
| (fn [] | ||
| (when (not= input-state controlled-input/init-state) | ||
| (set-input-state controlled-input/init-state))) | ||
| [current-address]) |
There was a problem hiding this comment.
I know when current-address changes, the input-state value in the callback would also be updated. But I think for correctness, we can use the value for input-state from the callback for setState in set-input-state.
That would be something like this:
(rn/use-effect
(fn []
(set-input-state #(if (= % controlled-input/init-state)
%
controlled-input/init-state)))
[current-address])What do you think?
There was a problem hiding this comment.
yeah makes sense, (just as a nit I'll use a fn so that I name the value prev-state so it's very clear. but overall sounds good to me! :)
thanks!
There was a problem hiding this comment.
actually looking at this, I'm not sure it really makes sense.
Both will set the state to init/state. I don't want to trigger any state change if the initial value is already set that way. What you are suggesting here is a bit different, no?
There was a problem hiding this comment.
When you return the same value that was in the prev state, it doesn't trigger an update, because the reference stays the same. So the only time a state update is triggered is in the second case where the previous value is not equal to init-state.
There was a problem hiding this comment.
true, I forgot about that. Anyway, I updated the function to take a different approach and it clears it safely now 👍
vkjr
approved these changes
May 20, 2024
briansztamfater
approved these changes
May 20, 2024
J-Son89
force-pushed
the
jc/fix-bugs
branch
2 times, most recently
from
d7f3e46
to
01a8258
Compare
85% of end-end tests have passedFailed tests (6)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
|
BalogunofAfrica
approved these changes
May 22, 2024
|
@mariia-skrypnyk - I see you are testing this pr. There was conflicts so I rebased and pushed the changes up 👍 |
|
Thanks @J-Son89 🙏🥹 |
33% of end-end tests have passedFailed tests (3)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hey @J-Son89 ! Thanks for your PR. |
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: #20100
In the sending flows, This pr updates the input amount page after switching account with the account switcher so that the input and related values will be flushed.
Screen.Recording.2024-05-20.at.11.17.15.mov
This pr also Hides the "Import Private Key" Flow from the user as the button was previously viewable and pressable but feature flagged.
Testing notes: this should also work on the bridging flows as well.