-
Notifications
You must be signed in to change notification settings - Fork 984
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
Use data from API to populate the buy assets sheet #20078
Conversation
Jenkins BuildsClick to see older builds (8)
|
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 🚀
:action-props {:alignment :flex-start | ||
:icon :i/external} | ||
:image :icon-avatar | ||
:image-props {:icon (status.resources/get-service-image :latamex)} |
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.
@ajayesivan - If it's not too much trouble, would you mind deleting the local images/assets for providers as we are using the URL to fetch images? This would save some space in the app.
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.
Sure
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.
nice work @ajayesivan! 🙌
417e171
to
2b6ab40
Compare
83% of end-end tests have passed
Failed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (43)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
|
2b6ab40
to
934bf15
Compare
79% of end-end tests have passed
Failed tests (9)Click to expandClass TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
|
Hi @ajayesivan, thanks for your work, everything looks good to me! In other words, don’t we expect that if the user long-tapped on ETH, then when going to Ramp, ETH will be pre-selected there? This sounds more advanced, I just want to make sure. Could you confirm that this is expected behavior, please? video_2024-05-21_11-39-19.mp4 |
Hi @qoqobolo, Thanks for testing this PR. As far as I understand, this is the expected behavior. We lack information on which platforms support which tokens. So, pre-populating the token when opening the sites may not be feasible. @J-Son89 or @dlipicar might be able to provide further clarification. |
934bf15
to
fd85895
Compare
Remove unsed code & resources
fd85895
to
74a6cd5
Compare
Yeap, we don't currently have the list of tokens supported by each provider and we don't pre-select a given asset when launching the provider's website. |
44% of end-end tests have passed
Failed tests (5)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestWalletMultipleDevice:
Passed tests (4)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
|
@ajayesivan @dlipicar thanks for confirming! PR can be merged. |
fixes #19836
Summary
This PR integrates and uses data from the status-go API for the buy assets sheet instead of using dummy data. The only UI change this PR introduces is the change of the title from 'Buy tokens' to 'Buy assets' as per the design.
Testing notes
Steps to test
Screenshot
status: ready