-
Notifications
You must be signed in to change notification settings - Fork 985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#18512] incomplete list of collectibles #18577
Conversation
Jenkins BuildsClick to see older builds (20)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 Nice work!
69% of end-end tests have passed
Failed tests (11)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Expected to fail tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Unassigned myself. Need to take more old PR instead of this fresh one |
487ef49
to
184215f
Compare
Hey @ulisesmac thank you for PR. Here are a found issues: ISSUE 1: [IOS] App crashes and unsmooth scrolling in the collectibles tabSteps:
Actual result:Unsmooth scrolling of collectibles, the app crashes during the process of scrolling collectibles video_collectibles.mp4Expected result:The scrolling of collectibles should be smooth, and the app should not crash during the scrolling process. Device:iPhone 11 Pro max, IOS 16 Logs: |
ISSUE 3: Collectible appears larger than others in the listSteps:
Actual Result:One of the collectibles appears larger than the others in the list. onecollectible.mp4Expected Result:All collectibles should be displayed at the same size in the collectibles list. Logs: |
thanks @VolodLytvynenko I'll solve it |
184215f
to
0efa026
Compare
Hey @VolodLytvynenko Thanks for testing.
on iOS the performance is slower, at least I can only test on a simulator and that's what I noticed. We can't do too much atm, the collectible component will be reworked, so maybe at that point we could check the collectible component performance and improve it. At least I couldn't replicate a crash with the fixes I did.
I already asked to wallet desktop team, to check what's happening. Status go only returns some images, not all 😢 we do the best we can with that info, as soon as it's fixed in status-go, we should see a difference here. For example: Status-go is just returning the collection image, not the collectible image: so that's what I'm showing, if status-go'd return the collectible, it'd be shown in the mobile app. Same for some missing collectibles 😞 So I'd say let's wait until it's fixed, I'll check with desktop what are the reasons for this to happen.
Fixed 😄 Additional notes
|
79% of end-end tests have passed
Failed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
|
@ulisesmac Thank you for clarification and fixes. No issues from my side. PR can be merged |
@ulisesmac I will ask design team about #18577 (comment). Perhaps this is intentional behavior |
@ulisesmac On mobile, we should show only collectibles on MVP. Here is a reference to the design answer: https://discord.com/channels/624634427930312714/852533718790570015/1200447531637821531 The PR can be merged, and we can revisit issue 2 only when collectible images fetching is implemented on Status Go. Thank you for investigating this |
fixes #18512
Summary
This PR shows collectibles having images in its
:collection-data
.So we are still not listing some collectibles, but the reason is status-go returns them without any image (sometimes just a name).
This PR also adds partial support to display SVG collectibles, but they are still not animated, so this PR is not closing:
Review notes
Future steps will be align with status-go team to investigate the reason of returning some collectibles empty.
Platforms
Steps to test
status: ready