Add collectible menu #18185
Add collectible menu #18185
Conversation
| @@ -269,7 +269,7 @@ | |||
| :max-cache-age-seconds max-cache-age-seconds} | |||
| request-params [request-id | |||
| [(chain/chain-id db)] | |||
| (map :address (:profile/wallet-accounts db)) | |||
| (keys (get-in db [:wallet :accounts])) | |||
There was a problem hiding this comment.
@mmilad75, here is a fix that enables collectibles retrieving.
Old wallet initialization was failed and :profile/wallet-accounts key is not longer populated, so now we need to get account information in a new way
There was a problem hiding this comment.
Apologies 🙏 for not spotting this yesterday when we removed it.
Jenkins BuildsClick to see older builds (8)
|
| (rf/dispatch [:wallet/request-collectibles | ||
| {:start-at-index 0 | ||
| :new-request? true}])) | ||
| [(count accounts)]) |
There was a problem hiding this comment.
@J-Son89, @ulisesmac, @briansztamfater, you may be interested:
Here I use use-effect because when user goes straight to wallet page, there is a moment when details about accounts aren't returned from status-go. And I've found kind of a problem: if you will try to use [accounts], or [@accoutns] - hook won't work properly and will call its function on every re-render. But with [(count accounts)] it works. So it looks like only simple values can be used as a conditional launch of use-effect hook.
I tried with atoms, reagent atom, clojure maps... all these kinds of values stated as condition trigger hook on every re-render(
If you know something about this behaviour - please share.
There was a problem hiding this comment.
Here I use
use-effectbecause when user goes straight to wallet page, there is a moment when details about accounts aren't returned fromstatus-go.
@vkjr - Perhaps we can move this dispatch on the success of fetching the accounts (wallet/get-accounts-success a correct place too)? We currently dispatch wallet/get-wallet-token to fetch token prices which depend on the account addresses.
This will also help us resolve a bug (undiscovered until now :)) where a newly added account doesn't display collectables unless we log in again.
There was a problem hiding this comment.
@J-Son89, @ulisesmac, @briansztamfater, you may be interested:
Here I use
use-effectbecause when user goes straight to wallet page, there is a moment when details about accounts aren't returned fromstatus-go. And I've found kind of a problem: if you will try to use[accounts], or[@accoutns]- hook won't work properly and will call its function on every re-render. But with[(count accounts)]it works. So it looks like only simple values can be used as a conditional launch ofuse-effecthook.
I tried with atoms, reagent atom, clojure maps... all these kinds of values stated as condition trigger hook on every re-render(
If you know something about this behaviour - please share.
Hey @vkjr
I have faced this same issue in the past.
The problem is that the hook dependencies are being compared in the JS side to check if the hook should be re-executed.
It means that in JS, numbers can be compared without any problem, (e.g. 4 = 4 always is true), but JS objects are compared by reference, so two objects that look exactly the same are different
(E.g.
#js{"0x123" #js{"a" 100}}
=
#js{"0x123" #js{"a" 100}}Always is false unless you store it in a let binding and compare the same var)
As far as I remember, the use-effect hook we are using transforms the dependencies vector from CLJS to JS, so it's creating new objects from clojure maps for each re-render.
Ok, enough of explanations. Let's give a solution.
We can rely on strings :) since they are compared by value.
So if we have a clojure map for accounts that looks like:
{"0x12..." {...}
"0x01..." {...}
"0xAB..." {...}}I'd do:
(-> @accounts keys sort)So that we get an ordered list of accounts that also works as hook dependency, if an account is added, the effect will detect a change and will re render, and I'm using sort because clojure hash maps aren't ordered (unless sorted-map is used), and we don't want to trigger renders just because of the order changed.
Note: maybe we'd need to add vec at the end of that -> if a vector must be passed to our use-effect dependency.
| {:render-fn quo/token-value | ||
| :data temp/tokens | ||
| :key :assets-list | ||
| :content-container-style {:padding-horizontal 8}}] |
There was a problem hiding this comment.
I'd say it's better to avoid inline styles, even if it's a single style
| (rf/dispatch [:wallet/request-collectibles | ||
| {:start-at-index 0 | ||
| :new-request? true}])) | ||
| [(count accounts)]) |
There was a problem hiding this comment.
Here I use
use-effectbecause when user goes straight to wallet page, there is a moment when details about accounts aren't returned fromstatus-go.
@vkjr - Perhaps we can move this dispatch on the success of fetching the accounts (wallet/get-accounts-success a correct place too)? We currently dispatch wallet/get-wallet-token to fetch token prices which depend on the account addresses.
This will also help us resolve a bug (undiscovered until now :)) where a newly added account doesn't display collectables unless we log in again.
| @@ -269,7 +269,7 @@ | |||
| :max-cache-age-seconds max-cache-age-seconds} | |||
| request-params [request-id | |||
| [(chain/chain-id db)] | |||
| (map :address (:profile/wallet-accounts db)) | |||
| (keys (get-in db [:wallet :accounts])) | |||
There was a problem hiding this comment.
Apologies 🙏 for not spotting this yesterday when we removed it.
| (def aval | ||
| (atom {:a 1 | ||
| :b 2})) | ||
|
|
||
| (reset! aval {:a 4}) |
There was a problem hiding this comment.
I guess this is intended for testing.
| :on-press #(js/alert "pressed")}] | ||
| :on-press #(rf/dispatch | ||
| [:show-bottom-sheet | ||
| {:content (fn [] [collectible-actions-sheet]) |
There was a problem hiding this comment.
| {:content (fn [] [collectible-actions-sheet]) | |
| {:content collectible-actions-sheet |
Yes, you are right, this is much better place) |
vkjr
force-pushed
the
collectible-menu
branch
from
6d52a65 to
c74874a
Compare
| {:keys [traits description]} collectible-data | ||
| chain-id (rf/sub [:wallet/last-collectible-chain-id])] | ||
| collection-data]} collectible | ||
| {:keys [traits name description]} collectible-data |
There was a problem hiding this comment.
I think name here can overshadow name keyword?
| {:padding-horizontal 8}) | ||
|
|
||
| (def home-container | ||
| {:margin-top (safe-area/get-top) |
There was a problem hiding this comment.
Can this safe area margin be applied in the navigation file?
There was a problem hiding this comment.
Probably yes, but I'd better make it the scope of other PR
| :on-press #(rf/dispatch | ||
| [:show-bottom-sheet | ||
| {:content collectible-actions-sheet | ||
| :theme nil}])}] |
There was a problem hiding this comment.
why we passing theme as nil here btw? 🤔
There was a problem hiding this comment.
Oh, thanks for noticing!) That was temporary
vkjr
force-pushed
the
collectible-menu
branch
from
6898fe9 to
b11681b
Compare
75% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (3)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (36)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
|
|
@vkjr thank you for the PR. Failed e2e are not PR related. Ready for merge in case manual testing is not needed. |
fixes #17323
Summary
status: ready