Use data from API to populate the buy assets sheet #20078
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajayesivan
requested review from
alwx,
briansztamfater,
smohamedjavid and
J-Son89
Jenkins BuildsClick to see older builds (8)
|
smohamedjavid
approved these changes
May 17, 2024
| :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.
@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.
ajayesivan
force-pushed
the
19836-integrate-crypto-on-ramps-api
branch
from
417e171
to
2b6ab40
Compare
83% of end-end tests have passedFailed 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:
|
briansztamfater
approved these changes
May 20, 2024
ajayesivan
force-pushed
the
19836-integrate-crypto-on-ramps-api
branch
from
2b6ab40
to
934bf15
Compare
79% of end-end tests have passedFailed 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. |
ajayesivan
force-pushed
the
19836-integrate-crypto-on-ramps-api
branch
from
934bf15
to
fd85895
Compare
Remove unsed code & resources
ajayesivan
force-pushed
the
19836-integrate-crypto-on-ramps-api
branch
from
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 passedFailed 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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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