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(earn): Add Crypto bottom sheet #5376

Merged
merged 20 commits into from
May 9, 2024
Merged

feat(earn): Add Crypto bottom sheet #5376

merged 20 commits into from
May 9, 2024

Conversation

finnian0826
Copy link
Contributor

@finnian0826 finnian0826 commented May 1, 2024

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:

  • 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

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:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

Copy link

codecov bot commented May 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.10%. Comparing base (d5e7740) to head (418d8c2).

Additional details and impacted files

Impacted file tree graph

@@            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              
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/earn/EarnAddCryptoBottomSheet.tsx 100.00% <100.00%> (ø)
src/tokens/TokenDetails.tsx 100.00% <100.00%> (ø)
src/tokens/TokenDetailsMoreActions.tsx 100.00% <ø> (ø)
src/tokens/types.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e7740...418d8c2. Read the comment docs.

@finnian0826 finnian0826 marked this pull request as ready for review May 6, 2024 22:18
@@ -2312,5 +2312,21 @@
"title": "More coming soon!"
}
}
},
"earnFlow": {
Copy link
Contributor

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

@@ -2312,5 +2312,21 @@
"title": "More coming soon!"
}
}
},
"earnFlow": {
"addCrypto": {
Copy link
Contributor

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

Suggested change
"addCrypto": {
"addCryptoBottomSheet": {

@@ -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',
Copy link
Contributor

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?

showSwap: ['arbitrum-sepolia'],
}
}),
getFeatureGate: jest.fn().mockReturnValue(true),
Copy link
Contributor

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?

Copy link
Contributor Author

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

},
visible:
isSwapEnabled &&
!!swappableFromTokens.find((tokenInfo) => tokenInfo.networkId === token.networkId),
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Send = 'Send',
Swap = 'Swap',
Add = 'Add',
Withdraw = 'Withdraw',
More = 'More',
Receive = 'Receive',
Copy link
Contributor

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?

import { TokenActionName } from 'src/tokens/types'
import { getTokenAnalyticsProps } from 'src/tokens/utils'

export default function EarnAddCrypto({
Copy link
Contributor

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

Comment on lines 147 to 154
...typeScale.labelMedium,
},
actionDetails: {
...typeScale.bodySmall,
},
title: {
...typeScale.labelLarge,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

add text color

Comment on lines 80 to 83
const { t } = useTranslation()
const { swappableFromTokens } = useSwappableTokens()
const cashInTokens = useCashInTokens()
const isSwapEnabled = useSelector(isAppSwapsEnabledSelector)
Copy link
Contributor

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

@satish-ravi
Copy link
Contributor

Also, some styling looks off compared to figma (title, gap between description and first action)

Comment on lines 2330 to 2332
"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!",
Copy link
Contributor

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

@@ -678,3 +678,7 @@ export enum PointsEvents {
export enum EarnEvents {
earn_cta_press = 'earn_cta_press',
}

export enum EarnEvents {
Copy link
Contributor

Choose a reason for hiding this comment

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

add to existing enum

@@ -1577,6 +1577,12 @@ interface EarnEventsProperties {
[EarnEvents.earn_cta_press]: undefined
}

interface EarnEventsProperties {
Copy link
Contributor

Choose a reason for hiding this comment

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

add to existing enum

@@ -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`,
Copy link
Contributor

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'),
Copy link
Contributor

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', () => {
Copy link
Contributor

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?

@@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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: [] })
Copy link
Contributor

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?

Copy link
Contributor Author

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

@finnian0826 finnian0826 enabled auto-merge May 9, 2024 16:21
@finnian0826 finnian0826 added this pull request to the merge queue May 9, 2024
Merged via the queue into main with commit 7d421cf May 9, 2024
16 checks passed
@finnian0826 finnian0826 deleted the finnian0826/act-1177 branch May 9, 2024 16:59
shottah pushed a commit to zed-io/kolektivo that referenced this pull request May 15, 2024
### 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)
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.

None yet

2 participants