-
Notifications
You must be signed in to change notification settings - Fork 97
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(earn): Add Crypto bottom sheet #5376
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5376 +/- ##
==========================================
+ Coverage 86.08% 86.10% +0.02%
==========================================
Files 739 740 +1
Lines 30089 30143 +54
Branches 5148 5152 +4
==========================================
+ Hits 25902 25955 +53
- Misses 3960 3961 +1
Partials 227 227
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -2312,5 +2312,21 @@ | |||
"title": "More coming soon!" | |||
} | |||
} | |||
}, | |||
"earnFlow": { |
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.
this (and a few other files) conflicts with main. I used earnStablecoin
as the key, but I like earnFlow
and updated it here https://github.com/valora-inc/wallet/pull/5394/files#diff-d3d67f4b3f8dbec345426f534268e40d9b087dfa5d83750165d63caf136cb24cR2329
locales/base/translation.json
Outdated
@@ -2312,5 +2312,21 @@ | |||
"title": "More coming soon!" | |||
} | |||
} | |||
}, | |||
"earnFlow": { | |||
"addCrypto": { |
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.
nit: to be more explicit
"addCrypto": { | |
"addCryptoBottomSheet": { |
src/analytics/Events.tsx
Outdated
@@ -673,3 +673,7 @@ export enum PointsEvents { | |||
points_screen_activity_try_again_press = 'points_screen_activity_try_again_press', | |||
points_screen_activity_fetch_more = 'points_screen_activity_fetch_more', | |||
} | |||
|
|||
export enum EarnEvents { | |||
earn_tap_add_crypto = 'earn_tap_add_crypto', |
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.
nit: maybe earn_tap_add_crypto_action
to be more explicit?
src/earn/EarnAddCrypto.test.tsx
Outdated
showSwap: ['arbitrum-sepolia'], | ||
} | ||
}), | ||
getFeatureGate: jest.fn().mockReturnValue(true), |
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.
is this required? Doesn't seem like the component was using getFeatureGate
. But if a subcomponent requires it, can we be explicit on what gates need to be true?
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.
no feature gates are required to be true, but some subcomponent uses feature gates (without this line the tests fail), so I changed it to set them false
src/earn/EarnAddCrypto.tsx
Outdated
}, | ||
visible: | ||
isSwapEnabled && | ||
!!swappableFromTokens.find((tokenInfo) => tokenInfo.networkId === token.networkId), |
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.
this should compare tokenId's right?
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.
I don't think so, swap should be enabled if we have a token that we can swap into USDC on arbitrum, so we want to check if there is a swappable token on the Arbitrum network, so I am comparing network IDs.
I guess I should also check that there is a match where the token ID is not USDC since we won't swap that for itself.
src/tokens/types.ts
Outdated
Send = 'Send', | ||
Swap = 'Swap', | ||
Add = 'Add', | ||
Withdraw = 'Withdraw', | ||
More = 'More', | ||
Receive = 'Receive', |
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.
should this just be Transfer
everywhere to match with the name in the designs?
src/earn/EarnAddCrypto.tsx
Outdated
import { TokenActionName } from 'src/tokens/types' | ||
import { getTokenAnalyticsProps } from 'src/tokens/utils' | ||
|
||
export default function EarnAddCrypto({ |
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.
naming nit: can we do EarnAddCryptoBottomSheet
(on the component, filename, etc)? I think we pretty much do it for every bottom sheet except maybe for TokenDetailsMoreActions
src/earn/EarnAddCrypto.tsx
Outdated
...typeScale.labelMedium, | ||
}, | ||
actionDetails: { | ||
...typeScale.bodySmall, | ||
}, | ||
title: { | ||
...typeScale.labelLarge, | ||
}, |
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.
add text color
src/earn/EarnAddCrypto.tsx
Outdated
const { t } = useTranslation() | ||
const { swappableFromTokens } = useSwappableTokens() | ||
const cashInTokens = useCashInTokens() | ||
const isSwapEnabled = useSelector(isAppSwapsEnabledSelector) |
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.
Do these work? I don't think we can use react hooks in a non hook fn and linter should catch it
Also, some styling looks off compared to figma (title, gap between description and first action) |
locales/base/translation.json
Outdated
"title": "Earn on your stablecoins", | ||
"subtitle": "Deposit today and earn returns", | ||
"description": "If you deposit <0></0>, you can get up to <1></1> at the end of the year!", |
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.
these can be removed, these have moved to earnFlow.cta
src/analytics/Events.tsx
Outdated
@@ -678,3 +678,7 @@ export enum PointsEvents { | |||
export enum EarnEvents { | |||
earn_cta_press = 'earn_cta_press', | |||
} | |||
|
|||
export enum EarnEvents { |
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.
add to existing enum
src/analytics/Properties.tsx
Outdated
@@ -1577,6 +1577,12 @@ interface EarnEventsProperties { | |||
[EarnEvents.earn_cta_press]: undefined | |||
} | |||
|
|||
interface EarnEventsProperties { |
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.
add to existing enum
src/analytics/docs.ts
Outdated
@@ -589,6 +589,7 @@ export const eventDocs: Record<AnalyticsEventType, string> = { | |||
[TransactionDetailsEvents.transaction_details_tap_check_status]: `When a user press 'Check status' on transaction details page`, | |||
[TransactionDetailsEvents.transaction_details_tap_retry]: `When a user press 'Retry' on transaction details page`, | |||
[TransactionDetailsEvents.transaction_details_tap_block_explorer]: `When a user press 'View on block explorer' on transaction details page`, | |||
[EarnEvents.earn_tap_add_crypto_action]: `When a user in the Earn flow enters an amount higher than their balance and chooses an option to add crypto`, |
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.
group with other earn events
}, | ||
{ | ||
name: TokenActionName.Transfer, | ||
title: t('earnFlow.addCryptoBottomSheet.actions.receive'), |
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.
receive -> transfer
}) | ||
|
||
describe('EarnAddCryptoBottomSheet', () => { | ||
it('Renders correct actions', () => { |
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.
can we also add a test that asserts the not visible case for actions?
src/analytics/Events.tsx
Outdated
@@ -677,4 +677,5 @@ export enum PointsEvents { | |||
|
|||
export enum EarnEvents { | |||
earn_cta_press = 'earn_cta_press', | |||
earn_tap_add_crypto_action = 'earn_tap_add_crypto_action', |
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.
earn_tap_add_crypto_action = 'earn_tap_add_crypto_action', | |
earn_add_crypto_action_press = 'earn_add_crypto_action_press', |
to be consistent with existing earn event
}), | ||
iconComponent: QuickActionsSend, | ||
onPress: () => { | ||
navigate(Screens.ExchangeQR, { flow: CICOFlow.CashIn, exchanges: [] }) |
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.
are exchanges supposed to be empty for the MVP?
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.
adding in a follow-up PR
### Description Creates new component for the Earn "Add Crypto" bottom sheet. This component should be used in the same way as the TokenDetailsMoreActions component [is used](https://github.com/valora-inc/wallet/blob/6df3859e7271db5e643aba11eda8cde19151135d/src/tokens/TokenDetails.tsx#L119). Decided to make a new component as to avoid adding extra props to decide copy, choose actions, and add an amount prop which would not be used in the TokenDetailsMoreActions case. Talked with product and Nitya and confirmed that: - On the QR code page, have title be "Deposit Crypto" like in the cash-in exchange case - Use "Arbitrum One" as the network name in copy Note: Price is not passed to the swap component, there is not support for that and based on discussion in quick sync it seems hard. ### Test plan Unit tests added. **Manual testing:** Bottom sheet: ![bottom-sheet](https://github.com/valora-inc/wallet/assets/140328381/0a029234-6bba-46dd-8512-6a73a12b660a) Tapping Buy: https://github.com/valora-inc/wallet/assets/140328381/2a8985ca-9ae2-41e1-b0f7-abed4071b723 Tapping Transfer: https://github.com/valora-inc/wallet/assets/140328381/61e38183-d641-43c8-9d2d-39af4236215d Tapping Swap: https://github.com/valora-inc/wallet/assets/140328381/393d00a6-41d7-49fc-a0f7-162e1ed15e71 Tapping ### Related issues - Fixes #ACT-1177 ### Backwards compatibility Yes, new component so not changing past stuff ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [X] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)
Description
Creates new component for the Earn "Add Crypto" bottom sheet. This component should be used in the same way as the TokenDetailsMoreActions component is used.
Decided to make a new component as to avoid adding extra props to decide copy, choose actions, and add an amount prop which would not be used in the TokenDetailsMoreActions case.
Talked with product and Nitya and confirmed that:
Note: Price is not passed to the swap component, there is not support for that and based on discussion in quick sync it seems hard.
Test plan
Unit tests added.
Manual testing:
Bottom sheet:
Tapping Buy:
add.mp4
Tapping Transfer:
transfer.mp4
Tapping Swap:
swap.mp4
Tapping
Related issues
Backwards compatibility
Yes, new component so not changing past stuff
Network scalability
If a new NetworkId and/or Network are added in the future, the changes in this PR will: