Fix usage of ratom to fix wallet share #18883
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 (44)
|
mohsen-ghafouri
approved these changes
Feb 18, 2024
J-Son89
reviewed
Feb 18, 2024
| @@ -74,7 +74,7 @@ | |||
| :emoji (:emoji account) | |||
| :on-multichain-press #(reset! wallet-type :multichain) | |||
| :on-legacy-press #(reset! wallet-type :legacy) | |||
| :on-settings-press #(open-preferences @selected-networks)}]]])))) | |||
| :on-settings-press #(open-preferences selected-networks)}]]])))) | |||
There was a problem hiding this comment.
Hmm, not particularly a fan of passing ratoms through to functions. Is this something we can avoid?
It seems better if a component can just take the value as a prop instead. Do we have a better approach for situations like this?
cc @ilmotta, @ulisesmac, @Parveshdhull
There was a problem hiding this comment.
Hi, In this particular scenario, we are modifying the state of current component based on what actions are performed on the next screen.
If we don't want to directly pass this ratom, we can instead pass the function that does the modification in the end (on-save)
There was a problem hiding this comment.
These functions are constant (:on-settings-press, :on-legacy-press and on-multichain-press) in terms of they always do the same.
Currently, each time the atoms selected-networks or wallet-type change, the component will regenerate the involved functions, but there's no need of it.
We can define them in the form-2 initializing section:
(defn wallet-qr-code-item-internal [props]
(let [{:keys [account width index]} props
selected-networks (reagent/atom [:ethereum :optimism :arbitrum])
wallet-type (reagent/atom :legacy)
on-legacy-press #(reset! wallet-type :legacy) ;; Here
on-settings-press #(open-preferences @selected-networks) ;; Here
...
]
(fn [...]
...)))So that we will pass the exact same object (function) to the child (quo/share-qr-code), it helps to avoid undesired rerenders.
@J-Son89 I think sometimes it's actually needed to deref an atom inside a callback function, but most of the times we can define that function once instead of on each rerender.
|
Also I think this fix is needed due to recent changes on the network preferences. Would appreciate that @smohamedjavid gets to check this before it goes to qa etc. |
smohamedjavid
approved these changes
Feb 19, 2024
96% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
|
|
@ibrkhalil hi! Can you please rebase your PR? |
Done |
|
Hi @ibrkhalil! Thanks for your PR! ISSUE 1: App crashes on Onboarding on Notifications step Steps:
Actual result: app crashes (10/10) Expected result: no crash |
Rebased again, Hopefully that fixes the issue. |
|
ISSUE 2: [iOS] Error on Wallet tab of Share screen Steps:
Actual result: Logs are attached. Expected result: no error |
62% of end-end tests have passedFailed tests (17)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (30)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
|
94% of end-end tests have passedFailed tests (1)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (16)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
|
|
Hi @ibrkhalil ! Please, feel free to merge! |
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 #18874
Summary
Fixes a typo related to wallet share screen.
status: ready