#[16978] Select collectible to send #18404
Conversation
ulisesmac
requested review from
shivekkhurana,
vkjr,
briansztamfater,
smohamedjavid,
J-Son89,
OmarBasem and
mmilad75
ulisesmac
force-pushed
the
16978-select-collectible
branch
from
7dcfaeb to
66699d8
Compare
| (let [collectible-list (rf/sub [:wallet/current-viewing-account-collectibles])] | ||
| [rn/view {:style {:flex 1}} | ||
| (case selected-tab | ||
| :assets [assets/view] | ||
| :collectibles [collectibles/view | ||
| {:collectibles collectible-list | ||
| :on-collectible-press (fn [id] | ||
| (rf/dispatch [:wallet/get-collectible-details id]) | ||
| (rf/dispatch [:navigate-to :wallet-collectible]))}] |
There was a problem hiding this comment.
I had to return to the approach of passing props to collectibles/view instead of just subscribing inside.
The reason is this is a general component, and make it able to handle the following by itself:
- All collectibles display (in wallet home)
- Collectibles only for the current account (when an account is picked)
- Collectibles being filtered (in the send asset screen)
Was hard, so I prefered to parameterize it.
Not as clean as before, but not too bad, and it's more reusable now.
There was a problem hiding this comment.
you could also just make the generic component and then make the various connected components that have the subs inside and handle the data connection etc. Looks good either way 👍
There was a problem hiding this comment.
Yeah, I think currently is ok, maybe for a later refactoring, we can consider it :)
| :on-collectible-press (fn [collectible-id] | ||
| (js/alert (str "Collectible to send: \n" | ||
| collectible-id | ||
| "\nNavigation not implemented yet")))}])) |
There was a problem hiding this comment.
Temp callback, I'll solve it in the following PR, which is show the details of the collectible to send.
This callback is triggered when the user press a collectible to send
Jenkins BuildsClick to see older builds (22)
|
| show-search-input? (or (= selected-tab :tab/assets) | ||
| (and (= selected-tab :tab/collectibles) | ||
| (seq unfiltered-collectibles)))] | ||
| [:<> | ||
| (when show-search-input? | ||
| [search-input search-text on-change-text]) |
There was a problem hiding this comment.
The search input is not always shown in the collectibles view, it's shown only when there are collectibles.
On the other hand, the assets tab is always showing the search input.
| :wallet/collectibles-per-account | ||
| :<- [:wallet] | ||
| (fn [wallet [_ address]] | ||
| (as-> wallet $ | ||
| (get-in $ [:accounts address :collectibles]) | ||
| (map (fn [{:keys [collectible-data] :as collectible}] | ||
| (assoc collectible :preview-url (preview-url collectible-data))) | ||
| $)))) | ||
| :wallet/current-viewing-account-collectibles | ||
| :<- [:wallet/current-viewing-account] | ||
| (fn [current-account] | ||
| (-> current-account :collectibles add-collectibles-preview-url))) |
There was a problem hiding this comment.
Removed the :wallet/collectibles-per-account sub and replaced it by :wallet/current-viewing-account-collectibles.
The reason is we weren't using the previous sub for specific accounts, it was always for the current account.
This one is simpler and makes our code cleaner.
| (and filtered? (empty? collectibles)) | ||
| [rn/view] |
There was a problem hiding this comment.
I believe this is intended to show text when no results match the user query.
We can add a note to it 👍
There was a problem hiding this comment.
imo we can let the code do this :)
(let [no-results-match-query? (and filtered? (empty? collectibles))]
(cond
no-results-match-query?
[rn/view]
...
| (let [collectibles (rf/sub [:wallet/current-viewing-account-collectibles-filtered search-text])] | ||
| [collectibles-tab/view | ||
| {:collectibles collectibles | ||
| :filtered? true |
There was a problem hiding this comment.
I believe we need to pass true to the filtered? prop only when there is an active search.
There was a problem hiding this comment.
if this goes beyond the prs scope let's track this for a separate issue to add filtering for search etc 👍
The sooner we have a happy path for sending a collectible the better :)
There was a problem hiding this comment.
I believe we need to pass true to the filtered? prop only when there is an active search.
@smohamedjavid exactly! it was a bug, I fixed it, thanks for noticing it, fixed!
| :permissions [empty-tab/view | ||
| {:title (i18n/label :t/no-permissions) | ||
| :description (i18n/label :t/no-collectibles-description) | ||
| :placeholder? true}] |
There was a problem hiding this comment.
I wonder do we have the illustration for this ready? if so can we track an issue to add this 🙏
There was a problem hiding this comment.
I've just asked designers, and they updated the designs, it seems it's stil a temporal solution, but it's easy to add, so will add it!
There was a problem hiding this comment.
Now it has been added
ulisesmac
force-pushed
the
16978-select-collectible
branch
from
66699d8 to
ef8ec03
Compare
well, I'm not sure yet, this is a change that is using the collectibles previously queried, so it supports testnet ones if our collectible listing (in home) supports them. I'd need to add some testing collectibles to test the sending, so I'll know 😆 |
ulisesmac
force-pushed
the
16978-select-collectible
branch
from
ef8ec03 to
cff9f2b
Compare
| {:title (i18n/label :t/nothing-found) | ||
| :description (i18n/label :t/try-to-search-something-else) | ||
| :image (resources/get-themed-image :no-collectibles theme)}]] | ||
|
|
| [empty-tab/view | ||
| {:title (i18n/label :t/no-collectibles) | ||
| :description (i18n/label :t/no-collectibles-description) | ||
| :image (resources/get-themed-image :no-collectibles theme)}] | ||
|
|
There was a problem hiding this comment.
These new lines are helpful when using a cond that has its "then" clausules beind the conditions. It's a common pattern in Clojure code
ulisesmac
force-pushed
the
16978-select-collectible
branch
from
cff9f2b to
829f26c
Compare
69% of end-end tests have passedFailed tests (12)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
|
ISSUE 2: Some collectibles are not fully visibleActual result:Collectibles are not visible. The empty space is shown instead collectibles.mp4Expected result:Collectibles are visible Logs:Devices:Android studio: Pixel XL API 33 |
Question 2:Should collectibles be displayed following the same principle as tokens? Currently, tokens are shown based on the testnet mode setting:
This is independent of whether the network is changed or not. However, for collectibles to be displayed, the 'mainnet' network needs to be selected and even if testnet mode is enabled, the mainnet collectibles are still shown instead of collectibles placed on testnet? |
About this, now I've confirmed these collectibles are being removed when received in the app, the reason is they don't have {:data-type 1,
:id {:contract-id {:chain-id 1,
:address "0x..."},
:token-id "12345"},
:collectible-data {:name "", ;; <- empty field
:image-url "", ;; <- empty field
:animation-url "", ;; <- empty field
:animation-media-type "", ;; <- empty field
:background-color ""} ;; <- empty field
;; Instead, collection-data is provided, but we are not looking at it right now:
:collection-data {:name " Name of the collection",
:slug "",
:image-url "https://some-url-to-the-image"},
:ownership [{:address "0x...",
:balance "1"}]} |
Thansk for testing this! This problem is happening also while we are in the home screen, so it's not very related to this PR. I checked the reason and it's about unsupported image formats mixed with links not having an extension, React Native just doesn't render them. and sometimes we need: We should open an issue to properly render all possible formats |
Yeah, I think collectibles should be shown in the same way, by looking at the code, we are only requesting the collectibles for the current network. |
|
Thank you so much for your review! I appreaciate your help a lot. I think these issues are important to solve, but they are out of the scope of this PR. These issues are happening all over the wallet, not only in the page that is listing the ones to transfer. So I'd suggest to open separate issues for them, I'll be happy to address them since I'm already familiar with this part of the code 😄 and in the meantime, we could focus on the screen to pick a collectible to send. wdyt? |
Yes. Agree with you. I rerun e2e one more time and let you know. Thank you |
60% of end-end tests have passedFailed tests (16)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (29)Click to expandClass TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
@ulisesmac Thank you for PR. No issues from my side. Other issues are not related to this PR and I will report them separately. PR ready to be merged |
Additionally, - Replace the sub `:wallet/collectibles-per-account` by `:wallet/current-viewing-account-collectibles` since it was only used for the current viewing account and simplified the code. - Refactor the select-asset view.
ulisesmac
force-pushed
the
16978-select-collectible
branch
from
0ca7668 to
5d8c349
Compare
fixes #16978
Summary
This PR shows the list of the collectibles to send and implements filtering capabilities:
figma link
Testing notes
You will need an account owning some collectibles, otherwise you will see an empty screen component.
Steps to test
My accountsCollectiblestabNow the existing collectibles are shown, or the empty-state component if there are no collectibles.
Also you are able to filter by the collectible name or collectible's collection name.
status: ready