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: add new Review component and apply it to SendConfirmation #6402

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open
22 changes: 12 additions & 10 deletions e2e/src/usecases/HandleDeepLinkSend.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ const launchDeepLink = async ({ url, newInstance = true }) => {
* @returns {Promise<string>}
*/
const getCryptoSymbol = async () => {
const sendAmountCryptoElement = await element(by.id('SendAmount')).getAttributes()
const sendAmountCryptoElement = await element(
by.id('SendConfirmationToken/Title')
).getAttributes()
return sendAmountCryptoElement.label.split(' ').at(-1)
}

Expand All @@ -41,11 +43,11 @@ export default HandleDeepLinkSend = () => {
jestExpect(cryptoSymbol).toBe('cUSD')

// Fiat amount should match value passed in deeplink
await waitFor(element(by.id('SendAmountFiat')))
await waitFor(element(by.id('SendConfirmationToken/Subtitle')))
.toHaveText('$0.01')
.withTimeout(10 * 1000)

await waitFor(element(by.id('DisplayName')))
await waitFor(element(by.id('SendConfirmationRecipient/Name/Title')))
.toHaveText('TestFaucet')
.withTimeout(10 * 1000)

Expand Down Expand Up @@ -87,7 +89,7 @@ export default HandleDeepLinkSend = () => {
await launchDeepLink({
url: deepLinks.withoutAddress,
})
await expect(element(by.id('SendAmount'))).not.toBeVisible()
await expect(element(by.id('SendConfirmationToken/Title'))).not.toBeVisible()
})
})

Expand All @@ -105,11 +107,11 @@ export default HandleDeepLinkSend = () => {
jestExpect(cryptoSymbol).toBe('cUSD')

// Fiat amount should match value passed in deeplink
await waitFor(element(by.id('SendAmountFiat')))
await waitFor(element(by.id('SendConfirmationToken/Subtitle')))
.toHaveText('$0.01')
.withTimeout(10 * 1000)

await waitFor(element(by.id('DisplayName')))
await waitFor(element(by.id('SendConfirmationRecipient/Name/Title')))
.toHaveText('TestFaucet')
.withTimeout(10 * 1000)

Expand All @@ -125,7 +127,7 @@ export default HandleDeepLinkSend = () => {

it('Then should error if no address provided', async () => {
await launchDeepLink({ url: deepLinks.withoutAddress, newInstance: false })
await expect(element(by.id('SendAmount'))).not.toBeVisible()
await expect(element(by.id('SendConfirmationToken/Title'))).not.toBeVisible()
})
})

Expand All @@ -138,11 +140,11 @@ export default HandleDeepLinkSend = () => {
jestExpect(cryptoSymbol).toBe('cUSD')

// Fiat amount should match value passed in deeplink
await waitFor(element(by.id('SendAmountFiat')))
await waitFor(element(by.id('SendConfirmationToken/Subtitle')))
.toHaveText('$0.01')
.withTimeout(10 * 1000)

await waitFor(element(by.id('DisplayName')))
await waitFor(element(by.id('SendConfirmationRecipient/Name/Title')))
.toHaveText('TestFaucet')
.withTimeout(10 * 1000)

Expand All @@ -158,7 +160,7 @@ export default HandleDeepLinkSend = () => {

it('Then should error if no address provided', async () => {
await openDeepLink(deepLinks.withoutAddress)
await expect(element(by.id('SendAmount'))).not.toBeVisible()
await expect(element(by.id('SendConfirmationToken/Title'))).not.toBeVisible()
})
})

Expand Down
6 changes: 3 additions & 3 deletions e2e/src/usecases/Send.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export default Send = () => {
})

it('Then should display correct recipient', async () => {
await expect(element(by.text(recipientAddressDisplay))).toBeVisible()
await expect(element(by.text(DEFAULT_RECIPIENT_ADDRESS))).toBeVisible()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addresses are now showed in full, not shortened

})

it('Then should be able to edit amount', async () => {
Expand All @@ -93,7 +93,7 @@ export default Send = () => {
timeout: 30_000,
tap: true,
})
let amount = await element(by.id('SendAmount')).getAttributes()
let amount = await element(by.id('SendConfirmationToken/Title')).getAttributes()
jestExpect(amount.text).toEqual('0.01 cEUR')
})

Expand Down Expand Up @@ -143,7 +143,7 @@ export default Send = () => {
})

it('Then should display correct recipient', async () => {
await expect(element(by.text(recipientAddressDisplay))).toBeVisible()
await expect(element(by.text(DEFAULT_RECIPIENT_ADDRESS))).toBeVisible()
})

it('Then should be able to send', async () => {
Expand Down
1 change: 1 addition & 0 deletions locales/base/translation.json
Original file line number Diff line number Diff line change
Expand Up @@ -2832,6 +2832,7 @@
"fiatPriceUnavailable": "Price unavailable",
"tokenDescription": "{{tokenName}} on {{tokenNetwork}}"
},
"totalPlusFees": "Total + Fees",
"gasFeeWarning": {
"title": "You need more {{tokenSymbol}} for gas fees",
"title_Dapp": "You have an insufficient gas token balance",
Expand Down
4 changes: 4 additions & 0 deletions src/account/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,7 @@ export function getPhoneNumberDetails(
}
}
}

export function formatShortenedAddress(address: string): string {
return `${address.slice(0, 6)}...${address.slice(-4)}`
}
2 changes: 1 addition & 1 deletion src/components/Avatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import * as React from 'react'
import { WithTranslation } from 'react-i18next'
import { StyleSheet, Text, TextStyle, View } from 'react-native'
import { defaultCountryCodeSelector } from 'src/account/selectors'
import { formatShortenedAddress } from 'src/account/utils'
import ContactCircle from 'src/components/ContactCircle'
import PhoneNumberWithFlag from 'src/components/PhoneNumberWithFlag'
import { formatShortenedAddress } from 'src/components/ShortenedAddress'
import { withTranslation } from 'src/i18n'
import { Recipient, getDisplayName } from 'src/recipients/recipient'
import { useSelector } from 'src/redux/hooks'
Expand Down
16 changes: 13 additions & 3 deletions src/components/ContactCircle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,11 @@ interface Props {
backgroundColor?: Colors
foregroundColor?: Colors
borderColor?: Colors
DefaultIcon?: React.ComponentType<{ foregroundColor?: Colors; backgroundColor?: Colors }>
DefaultIcon?: React.ComponentType<{
color?: Colors
backgroundColor?: Colors
size?: number
}>
}

const DEFAULT_ICON_SIZE = 40
Expand All @@ -30,7 +34,7 @@ function ContactCircle({
backgroundColor,
foregroundColor,
borderColor,
DefaultIcon = ({ foregroundColor }) => <User color={foregroundColor} />,
DefaultIcon = ({ color, size = 20 }) => <User size={size} color={color} />,
Copy link
Collaborator

Choose a reason for hiding this comment

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

why apply a size to User when it is overwritten when the DefaultIcon is rendered later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's indeed redundant, will remove!

}: Props) {
const address = recipient.address
const iconBackgroundColor = backgroundColor ?? getAddressBackgroundColor(address || '0x0')
Expand Down Expand Up @@ -65,7 +69,13 @@ function ContactCircle({
)
}

return <DefaultIcon foregroundColor={fontColor} backgroundColor={iconBackgroundColor} />
return (
<DefaultIcon
size={iconSize / 1.625}
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 like using magic numbers but in this case this is the exact proportion as per the new designs (which I think must be applied across the codebase for consistency)

color={fontColor}
backgroundColor={iconBackgroundColor}
/>
)
}

return (
Expand Down
147 changes: 147 additions & 0 deletions src/components/Review.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import { render } from '@testing-library/react-native'
import React from 'react'
import { type Recipient } from 'src/recipients/recipient'
import { typeScale } from 'src/styles/fonts'
import {
Review,
ReviewContent,
ReviewDetailsItem,
ReviewSummaryItem,
ReviewSummaryItemContact,
} from './Review'

describe('Review', () => {
it('uses the custom headerAction if provided', async () => {
const tree = render(
<Review title="Custom HeaderAction" headerAction={<>Custom Left Action</>} testID="Review">
<ReviewContent>
<></>
</ReviewContent>
</Review>
)

expect(tree.getByTestId('Review')).toHaveTextContent('Custom Left Action')
})
})

describe('ReviewSummaryItem', () => {
it('renders the title and optional subtitle', () => {
const tree = render(
<ReviewSummaryItem
header="Item Header"
title="Item Title"
subtitle="Item Subtitle"
icon={<>Item Icon</>}
testID="MyItem"
/>
)

// Check if header, title, and subtitle all exist
expect(tree.getByTestId('MyItem/Header')).toHaveTextContent('Item Header')
expect(tree.getByTestId('MyItem/Title')).toHaveTextContent('Item Title')
expect(tree.getByTestId('MyItem/Subtitle')).toHaveTextContent('Item Subtitle')
expect(tree.getByTestId('MyItem')).toHaveTextContent('Item Icon')
})

it('does not render subtitle if not provided', () => {
const tree = render(
<ReviewSummaryItem header="Header" title="Title" icon={<></>} testID="NoSubtitleItem" />
)
expect(tree.queryByTestId('NoSubtitleItem/Subtitle')).toBeNull()
})
})

describe('ReviewSummaryItemContact', () => {
it('displays name + phone if recipient has a name and phone number', () => {
const recipient = {
name: 'John Doe',
displayNumber: '+111111111',
e164PhoneNumber: '+222222222',
} as Recipient
const tree = render(
<ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
)

expect(tree.getByTestId('ContactItem/Name/Header')).toHaveTextContent('Contact')
expect(tree.getByTestId('ContactItem/Name/Title')).toHaveTextContent('John Doe')
expect(tree.getByTestId('ContactItem/Name/Subtitle')).toHaveTextContent('+111111111')
})

it('displays only displayNumber phone if name is not available', () => {
const recipient = {
displayNumber: '+111111111',
e164PhoneNumber: '+222222222',
} as Recipient
const tree = render(
<ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
)

// This means phone is the title, no subtitle
expect(tree.getByTestId('ContactItem/Phone/Title')).toHaveTextContent('+111111111')
expect(tree.queryByTestId('ContactItem/Phone/Subtitle')).toBeNull()
})

it('displays only e164PhoneNumber phone if name and displayNumber are not available', () => {
const recipient = {
e164PhoneNumber: '+222222222',
} as Recipient
const tree = render(
<ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
)

// This means phone is the title, no subtitle
expect(tree.getByTestId('ContactItem/Phone/Title')).toHaveTextContent('+222222222')
expect(tree.queryByTestId('ContactItem/Phone/Subtitle')).toBeNull()
})

it('displays address if name/phone not available', () => {
const recipient = {
address: '0x123456789',
} as Recipient
const tree = render(
<ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
)

expect(tree.getByTestId('ContactItem/Address/Title')).toHaveTextContent('0x123456789')
})

it('displays "unknown" if no name/phone/address exist', () => {
const recipient = {} as Recipient
const tree = render(
<ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
)

// "t('unknown')" returns "mockedTranslation:unknown" as per our mock
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait is this true? i don't see the string mockedTranslation in any searches...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kathaypacific ah sorry, that was initial chatgpt generated comment string that I missed to remove 😅

expect(tree.getByTestId('ContactItem/Unknown/Title')).toHaveTextContent('unknown')
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

i wonder if these tests would benefit from a it.each, since the tests content is the same but only the recipient and text content changes? also fine to leave it like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They differ ever so slightly but I might change it. I'll look into it.

})

describe('ReviewDetailsItem', () => {
it('renders loading skeleton if isLoading is true', () => {
const tree = render(
<ReviewDetailsItem
isLoading
label="Loading Label"
value="Should not show"
testID="LoadingItem"
/>
)

expect(tree.getByTestId('LoadingItem/Loader')).toBeTruthy()
// The value text is not displayed
expect(tree.queryByText('Should not show')).toBeNull()
})

it('renders value text if isLoading is false', () => {
const tree = render(<ReviewDetailsItem label="Label" value="Value" testID="DetailsItem" />)
expect(tree.queryByTestId('DetailsItem/Loader')).toBeNull()
expect(tree.getByTestId('DetailsItem/Value')).toHaveTextContent('Value')
})

it('applies bold variant if specified', () => {
const tree = render(
<ReviewDetailsItem label="Bold Label" value="Bold Value" variant="bold" testID="BoldItem" />
)
expect(tree.getByTestId('BoldItem/Label')).toHaveStyle(typeScale.labelSemiBoldMedium)
})
})
Loading
Loading