Collectible details page #17520

vkjr · 2023-10-04T12:51:04Z

Summary

Implemented a collectibles list on an accounts page and the collectible details page.
Also few small changes were made:

in page-nav component middle part now can have opacity, to use this with animation
scroll-page now receives more configuration to its internal page-nav component and uses it instead of providing own logo/title
fixed issues on quo preview screens that I ran into

Review notes

Pages use mock data and do not have prepared re-frame subscriptions. They will be implemented as a next step when connecting to backend.

Designs: https://www.figma.com/file/HKncH4wnDwZDAhc4AryK8Y/Wallet-for-Mobile?type=design&node-id=1355-300817&mode=dev

status: ready

status-im-auto · 2023-10-04T12:58:31Z

Jenkins Builds

Click to see older builds (9)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`60859f6`	#1	2023-10-04 12:58:30	~7 min	`ios`	📱`ipa` 📲
✔️	`60859f6`	#1	2023-10-04 12:59:17	~7 min	`android`	🤖`apk` 📲

✔️	`6eeff34`	#2	2023-10-04 13:06:36	~6 min	`android`	🤖`apk` 📲
✔️	`6eeff34`	#2	2023-10-04 13:07:50	~7 min	`ios`	📱`ipa` 📲
✔️	`6eeff34`	#2	2023-10-04 13:08:23	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`6eeff34`	#2	2023-10-04 13:10:00	~9 min	`tests`	📄`log`

❌	`94dca62`	#3	2023-10-06 10:06:37	~2 min	`tests`	📄`log`
✔️	`94dca62`	#3	2023-10-06 10:09:59	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`94dca62`	#3	2023-10-06 10:10:24	~6 min	`android`	🤖`apk` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`8f5a751`	#4	2023-10-06 10:18:31	~6 min	`android-e2e`	🤖`apk` 📲
✔️	`8f5a751`	#4	2023-10-06 10:18:32	~6 min	`android`	🤖`apk` 📲
✔️	`8f5a751`	#4	2023-10-06 10:19:56	~7 min	`ios`	📱`ipa` 📲
✔️	`8f5a751`	#4	2023-10-06 10:21:05	~8 min	`tests`	📄`log`

✔️	`66e124c`	#5	2023-10-09 10:55:48	~7 min	`ios`	📱`ipa` 📲
✔️	`66e124c`	#5	2023-10-09 10:57:45	~9 min	`android-e2e`	🤖`apk` 📲
✔️	`66e124c`	#5	2023-10-09 10:58:03	~9 min	`android`	🤖`apk` 📲
✔️	`66e124c`	#5	2023-10-09 10:58:39	~10 min	`tests`	📄`log`

J-Son89 · 2023-10-04T20:33:27Z

src/quo2/components/wallet/wallet_overview/style.cljs

@@ -4,7 +4,6 @@
 (def container-info
  {:padding-horizontal 20
   :padding-top        12
-   :flex-grow          1


this is definitely safe? 🤔

Did you check if it looks fine in quo2 and if used in other screens?

No, it doesn't look fine, thanks!
That flex-grow was causing problems on wallet accounts page since it is used on the same page with flat-list. And when there are not enough items in a flat-list, wallet-overview grows in width and that results in jumps when user switches between "assets" and "collectibles" tabs.
I will return flex-grow and will put wallet-overview in an enclosing view of fixed height. It is okay when it grows horizontally, but vertical growth isn't really needed for this component.

I think we shouldn't be using fixed height. Most of the time flex-box is sufficient

J-Son89 · 2023-10-04T20:34:51Z

src/status_im2/common/scroll_page/view.cljs

@@ -54,17 +54,6 @@
               :top      0
               :left     0
               :right    0}}
-      (when logo


it's okay to delete this? this component is used in the communities overview page? or am I missing something and this code is elsewhere now?

scroll-page uses page-nav internally but before it was using it with :type :no-title and showing title/logo on top of that component. Since page-nav know how to render title and logo, I got rid of logo in title directly in scroll-page and added the possibility to configure internal page-nav instead. Here is how communities overview now configured:

status-mobile/src/status_im2/contexts/communities/overview/view.cljs

Line 329 in 6eeff34

:page-nav-props {:type :community

Thanks for the explanation! 🙏

J-Son89 · 2023-10-04T20:36:23Z

src/status_im2/contexts/communities/overview/view.cljs

+          :name             name
+          :on-scroll        #(reset! scroll-height %)
+          :navigate-back?   true
+          :background-color (colors/theme-colors colors/white colors/neutral-95)


is it possible to add the theme param here 🙏

I commented usage of:background-color in scroll-page on both - community overview and discover pages and don't see any visual changes.
So I think we can get rid of background-color at all here. Transparent scroll-page is good enough)
In case we will need some coloring later, it probably would be more idiomatic to create a separate component through with-theme, because now scroll-page has no access to theme.

J-Son89 · 2023-10-04T20:38:42Z

src/status_im2/contexts/wallet/collectible/view.cljs

+                       :right-side  [{:icon-name :i/options
+                                      :on-press  #(js/alert "pressed")}]
+                       :picture     image}}
+     nil


what is nil being set on here? it's required or we can make this function/component multi-arity instead? 🤔

Multi-arity doesn't work well with re-frame form-2 components, because they called only once and result is cached. But I passed sticky header component as a prop instead of making it an argument and that works well.

J-Son89 · 2023-10-04T20:40:50Z

src/status_im2/contexts/wallet/collectible/view.cljs

+          :status              :default
+          :size                :default
+          :title               (i18n/label :t/account-title)
+          :subtitle            "Collectibles vault"


should we put all this info in the temp file? either way is okay for me 👍

J-Son89 · 2023-10-04T20:48:06Z

src/status_im2/contexts/wallet/collectible/view.cljs

+          :status      :default
+          :size        :default
+          :title       (i18n/label :t/network)
+          :subtitle    "Mainnet"}]]]


I think mainnet should be an i18n label right? it's a bit more permanent

J-Son89 · 2023-10-04T20:48:57Z

src/status_im2/contexts/wallet/collectible/view.cljs

+                                     {:description :default
+                                      :card?       true
+                                      :status      :default
+                                      :size        :default


do we use "size" default on that component? it's a bit older or something?

data-item component has sizes "small" and "default" by Figma design and as far as I can tell from quo implementation and preview.

J-Son89 · 2023-10-04T20:49:47Z

src/status_im2/contexts/wallet/home/style.cljs

@@ -5,6 +5,9 @@
   :padding-top        8
   :padding-bottom     12})

+(def accounts-container
+  {:height 112})


should we just use flex instead?

Using flex will affect the height of the component and it will change every time we change assets/collectibles tab (because we are switching between different flatlists and they take different amounts of space depending on children).
That change will result in the whole tabbar jumping when navigating tabs.
Btw, I don't remember discussing any problem with having fixed height, especially on the screens like this, where flat-list at the bottom will eat up all the remaining space. I believe fixed with is more problematic. But maybe I miss something?

Yeah you're right!

J-Son89 · 2023-10-04T20:50:01Z

src/status_im2/contexts/wallet/home/view.cljs

@@ -46,7 +46,7 @@
 (defn view
  []
  (let [top          (safe-area/get-top)
-        selected-tab (reagent/atom (:id (first tabs-data)))]
+        selected-tab (reagent/atom (:id (second tabs-data)))]


intentional?

J-Son89 · 2023-10-04T20:52:43Z

src/status_im2/contexts/wallet/home/view.cljs

                         :content-container-style {:padding-horizontal 8}}]
-         :collectibles [quo/empty-state


curious, I guess we should still keep the empty-state right? as it is something we intent to keep when the user has none. Maybe we pass the mock data from higher up so this code can be left in place? 🤔

J-Son89 · 2023-10-04T20:53:13Z

src/status_im2/navigation/screens.cljs

@@ -246,7 +246,7 @@
     :component wallet-address-watch/view}

    {:name      :wallet-collectibles


should we change this name too?? to :wallet-collectible ?

OmarBasem · 2023-10-05T04:14:38Z

src/quo2/components/wallet/wallet_overview/style.cljs

@@ -4,7 +4,6 @@
 (def container-info
  {:padding-horizontal 20
   :padding-top        12
-   :flex-grow          1


Did you check if it looks fine in quo2 and if used in other screens?

OmarBasem · 2023-10-05T04:19:18Z

src/status_im2/contexts/wallet/collectible/view.cljs

+
+      [rn/image
+       {:source image
+        :style  style/preview}]
+


empty lines

OmarBasem · 2023-10-05T04:19:45Z

src/status_im2/contexts/wallet/collectible/view.cljs

+         {:weight :semi-bold
+          :size   :paragraph-1}
+         description]]]
+


OmarBasem · 2023-10-05T04:19:53Z

src/status_im2/contexts/wallet/collectible/view.cljs

+         :size            40
+         :icon-left       :i/opensea}
+        (i18n/label :t/opensea)]]
+


OmarBasem · 2023-10-05T04:19:58Z

src/status_im2/contexts/wallet/collectible/view.cljs

+                      {:id                  :about
+                       :label               (i18n/label :t/about)
+                       :accessibility-label :about-tab}]}]
+


EL, and there is a few more in this file

If component is too long, then would be good to break it up into smaller pieces

vkjr · 2023-10-06T11:25:42Z

@J-Son89, @OmarBasem, fixed the review notes, could you please take a look again?)

J-Son89

nice work @vkjr! 🥳

status-im-auto · 2023-10-09T12:02:27Z

84% of end-end tests have passed

Total executed tests: 43
Failed tests: 7
Passed tests: 36

IDs of failed tests: 702732,703495,703297,703202,702786,702731,702808

Failed tests (7)

Click to expand

Rerun failed tests

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_pin_messages, id: 702731
`Test is not run, e2e blocker` [[reason: [NOTRUN] Pin feature is in development]]

Class TestGroupChatMultipleDeviceMergedNewUI:

1. test_group_chat_pin_messages, id: 702732
`Test is not run, e2e blocker` [[reason: [NOTRUN] Pin feature is in development]]
2. test_group_chat_mute_chat, id: 703495
Device 2: Click until `ChatMessageInput` by `accessibility id`: `chat-message-input` will be presented Device 2: Looking for a message by text: Chat is unmuted now `critical/chats/test_group_chat.py:473: in test_group_chat_mute_chat self.errors.verify_no_errors() base_test_case.py:191: in verify_no_errors pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))])) New messages counter near chats tab button is 1 after unmute, but should be 2` Device sessions Device 1: Steps, video, logs Failure screenshot Device 2: Steps, video, logs Failure screenshot Device 3: Steps, video, logs Failure screenshot
3. test_group_chat_send_image_save_and_share, id: 703297
# STEP: Member_2 checks image message Device 3: Looking for a message by text: test image critical/chats/test_group_chat.py:229: in test_group_chat_send_image_save_and_share self.chats[2].chat_element_by_text(image_description).wait_for_visibility_of_element(60) ../views/base_element.py:139: in wait_for_visibility_of_element raise TimeoutException( Device 3: ChatElementByText by xpath:`//*[starts-with(@text,'test image')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element Device sessions Device 1: Steps, video, logs Failure screenshot Device 2: Steps, video, logs Failure screenshot Device 3: Steps, video, logs Failure screenshot
4. test_group_chat_reactions, id: 703202
Device 3: Adding one more 'thumbs-up' reaction or removing an added one Device 3: Find `Button` by `accessibility id`: `emoji-reaction-2` critical/chats/test_group_chat.py:112: in test_group_chat_reactions self.chats[2].add_remove_same_reaction(message=message, emoji="thumbs-up") ../views/chat_view.py:1076: in add_remove_same_reaction element.click() ../views/base_element.py:90: in click self.find_element().click() ../views/base_element.py:79: in find_element raise NoSuchElementException( Device 3: Button by accessibility id: `emoji-reaction-2` is not found on the screen; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception Device sessions Device 1: Steps, video, logs Failure screenshot Device 2: Steps, video, logs Failure screenshot Device 3: Steps, video, logs Failure screenshot
5. test_group_chat_offline_pn, id: 702808
Device 3: Looking for a message by text: message from old member Device 3: Looking for a message by text: message from new member `critical/chats/test_group_chat.py:309: in test_group_chat_offline_pn self.errors.verify_no_errors() base_test_case.py:191: in verify_no_errors pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))])) Messages PN was not fetched from offline E message from old member if not shown for device 2` [[Data delivery issue]] Device sessions Device 1: Steps, video, logs Failure screenshot Device 2: Steps, video, logs Failure screenshot Device 3: Steps, video, logs Failure screenshot

Class TestCommunityMultipleDeviceMergedTwo:

1. test_community_mentions_push_notification, id: 702786

Device 2: Getting PN by 'user_2'
Device 2: Looking for a message by text: user_2

critical/chats/test_public_chat_browsing.py:874: in test_community_mentions_push_notification
    self.errors.verify_no_errors()
base_test_case.py:191: in verify_no_errors
    pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
 Can not edit a message with a mention

Device sessions

Device 1:

Device 2:

Passed tests (36)

Click to expand

Class TestActivityMultipleDevicePR:

1. test_navigation_jump_to, id: 702936
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestCommunityOneDeviceMerged:

1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions Device 1: Steps, video, logs
2. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions Device 1: Steps, video, logs
3. test_community_discovery, id: 703503
Device sessions Device 1: Steps, video, logs
4. test_community_undo_delete_message, id: 702869
Device sessions Device 1: Steps, video, logs
5. test_community_navigate_to_channel_when_relaunch, id: 702846
Device sessions Device 1: Steps, video, logs
6. test_community_mute_community_and_channel, id: 703382
Device sessions Device 1: Steps, video, logs

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_1_1_chat_text_message_delete_push_disappear, id: 702733
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_1_1_chat_push_emoji, id: 702813
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
4. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
5. test_1_1_chat_edit_message, id: 702855
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
6. test_1_1_chat_send_image_save_and_share, id: 703391
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
7. test_1_1_chat_message_reaction, id: 702730
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestCommunityMultipleDeviceMerged:

1. test_community_several_images_send_reply, id: 703194
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_community_one_image_send_reply, id: 702859
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_community_emoji_send_copy_paste_reply, id: 702840
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
4. test_community_mark_all_messages_as_read, id: 703086
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
5. test_community_contact_block_unblock_offline, id: 702894
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
6. test_community_message_delete, id: 702839
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
7. test_community_message_send_check_timestamps_sender_username, id: 702838
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
8. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
9. test_community_message_edit, id: 702843
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
10. test_community_unread_messages_badge, id: 702841
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

1. test_1_1_chat_delete_via_long_press_relogin, id: 702784
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_1_1_chat_mute_chat, id: 703496
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestActivityCenterContactRequestMultipleDevicePR:

1. test_activity_center_contact_request_accept_swipe_mark_all_as_read, id: 702851
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_activity_center_contact_request_decline, id: 702850
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestCommunityMultipleDeviceMergedTwo:

1. test_community_markdown_support, id: 702809
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_community_hashtag_links_to_community_channels, id: 702948
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_community_leave, id: 702845
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestActivityMultipleDevicePRTwo:

1. test_activity_center_mentions, id: 702957
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_activity_center_admin_notification_accept_swipe, id: 702958
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestGroupChatMultipleDeviceMergedNewUI:

1. test_group_chat_join_send_text_messages_push, id: 702807
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs Device 3: Steps, video, logs

vkjr self-assigned this Oct 4, 2023

vkjr force-pushed the collectibles-list branch from 60859f6 to 6eeff34 Compare October 4, 2023 12:59

vkjr requested review from briansztamfater, J-Son89, OmarBasem and mmilad75 October 4, 2023 13:03

J-Son89 reviewed Oct 4, 2023

View reviewed changes

OmarBasem reviewed Oct 5, 2023

View reviewed changes

vkjr force-pushed the collectibles-list branch from 6eeff34 to 94dca62 Compare October 6, 2023 10:03

vkjr requested review from J-Son89 and OmarBasem October 6, 2023 10:10

J-Son89 approved these changes Oct 9, 2023

View reviewed changes

vkjr added 3 commits October 9, 2023 11:47

Collectible details page

ce1e543

review notes

34475bd

lint fix

66e124c

vkjr force-pushed the collectibles-list branch from 8f5a751 to 66e124c Compare October 9, 2023 10:47

OmarBasem approved these changes Oct 9, 2023

View reviewed changes

vkjr merged commit 1770ff2 into develop Oct 9, 2023
2 checks passed

vkjr deleted the collectibles-list branch October 9, 2023 13:18

vkjr mentioned this pull request Oct 9, 2023

Implement collectibles list on accounts page #17298

Closed

1 task

		:content-container-style {:padding-horizontal 8}}]
		:collectibles [quo/empty-state

		@@ -246,7 +246,7 @@
		:component wallet-address-watch/view}

		{:name :wallet-collectibles

Collectible details page #17520

Collectible details page #17520

Conversation

vkjr commented Oct 4, 2023 • edited Loading

Summary

Review notes

status-im-auto commented Oct 4, 2023 • edited Loading

Jenkins Builds

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkjr commented Oct 6, 2023

J-Son89 left a comment

Choose a reason for hiding this comment

status-im-auto commented Oct 9, 2023

84% of end-end tests have passed

Failed tests (7)

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestGroupChatMultipleDeviceMergedNewUI:

Class TestCommunityMultipleDeviceMergedTwo:

Passed tests (36)

Class TestActivityMultipleDevicePR:

Class TestCommunityOneDeviceMerged:

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestCommunityMultipleDeviceMerged:

Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:

Class TestActivityCenterContactRequestMultipleDevicePR:

Class TestCommunityMultipleDeviceMergedTwo:

Class TestActivityMultipleDevicePRTwo:

Class TestGroupChatMultipleDeviceMergedNewUI:

vkjr commented Oct 4, 2023 •

edited

Loading

status-im-auto commented Oct 4, 2023 •

edited

Loading