Skip to content
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

feat(extension) add FOOR buy ADA button to Banxa [LW-5104],[LW-10213] #1016

Merged
merged 22 commits into from
May 21, 2024

Conversation

tommayeliog
Copy link
Contributor

@tommayeliog tommayeliog commented Apr 3, 2024

Checklist


Proposed solution

But ADA button added for Tokens page on the side bar (or under portfolio ballance, when no sidebar visible).
Hidden behind a feature flag USE_BANXA_TOPUP
If user agrees T&C and click "Continue" a new Tab opens with Banxa url.

PostHog actions added - LW-10213

Testing

For testing please change locally USE_BANXA_TOPUP=true

Screenshots

Uploading Screenshot 2024-05-10 at 17.01.25.png…

Copy link

github-actions bot commented Apr 3, 2024

Allure Report

allure-report-publisher generated test report!

smokeTests: ✅ test report for a356d56e

passed failed skipped flaky total result
Total 30 0 0 0 30

@coveralls
Copy link

Coverage Status

coverage: 50.615% (+0.06%) from 50.56%
when pulling 348f1bc on feat/LW-5104-foor-buy-ada-button-modal
into 7071759 on main.

height: 20px;
}
.badgeCaption {
color: var(--White, #FFF);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please use proper color from _light/_dark.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vetalcore I've change it to --color-white. This color will always be white regardless of theme because it's on specific background

apps/browser-extension-wallet/src/lib/translations/en.json Outdated Show resolved Hide resolved
import { useTranslation } from 'react-i18next';
import { BANXA_URL } from './config';

export const TopUpWalletButton = (): React.ReactElement => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a button? Maybe we could leave TopUpWallet as a name because it's a feature with a button and dialog. You can come up with a better name ofc 😄

On the other hand I'd probably leave the button/dialog separately and integrate the logic in some sort of TopUpWalletCardContainer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @przemyslaw-wlodek, yes, it's a button and a modal. It's not stated on the JIRA ticket but maybe at some point a new requirement will be added for the modal to be visible just once. Then Button will be much more relevant. TBO I don't know - maybe TopUpWalletButtonConfirmation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider TopUpWallet then 😃 WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @przemyslaw-wlodek, I'm not 100% sure :) but it just because the feature has been named with the same name (TopUpWallet), and this "button + modal" is just one of the components, however I'm happy to rename it.

@pczeglik-iohk
Copy link
Contributor

@tommayeliog when you will have this feature ready to be merge, please remember to put JIRA ticket ID like LW-xxxx into the commit message. This helps to identify JIRA task by the commit message. You can check commit from main to get some inspiration ;) https://github.com/input-output-hk/lace/commits/main/

Copy link

sonarcloud bot commented Apr 4, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@przemyslaw-wlodek przemyslaw-wlodek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left 1 minor comment about naming. Thank you @tommayeliog! GJ 🚀 Feel free to proceed when you resolve other comments.

@tommayeliog tommayeliog changed the title feat(extension) add FOOR buy ADA button to Banxa feat(extension) add FOOR buy ADA button to Banxa [LW-5104],[LW-10213] Apr 8, 2024
@tommayeliog tommayeliog removed the request for review from mchappell May 20, 2024 10:33
tommayeliog and others added 2 commits May 20, 2024 16:12
…s/TopUpWallet/TopUpWallet.module.scss

Co-authored-by: Michael Chappell <[email protected]>
Signed-off-by: Tom Mayel <[email protected]>
…s/TopUpWallet/TopUpWallet.module.scss

Co-authored-by: Michael Chappell <[email protected]>
Signed-off-by: Tom Mayel <[email protected]>
Copy link

sonarcloud bot commented May 20, 2024

@tommayeliog tommayeliog merged commit 5e0a649 into main May 21, 2024
12 checks passed
@tommayeliog tommayeliog deleted the feat/LW-5104-foor-buy-ada-button-modal branch May 21, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants