feat: add new Review component and apply it to SendConfirmation #6402

sviderock · 2025-01-09T09:35:05Z

This PR adds a new reusable Review component to be used for all the review screens and applies it to the SendConfirmation screen. (design)

Review component is implemented with a bunch of composable components which might look too abstract (e.g. a bunch of components are just a single View with styles and children) but they provide a few benefits:

all the styles are incapsulated inside of the Review.tsx file so there are no need to copy-paste all the styles (even small ones like gap: 16) to each review screen
the whole structure of the Review screen is purely JSX so there is a clear vision of how many things is rendered and what those things are specifically
no need to move items to an array using useMemo (e.g. if we were to implement ReviewDetails as a component that accepts an array prop for all the children items)
we can remove a lot of tests from each individual screen as all the review-related things will be tested from within Review.test.tsx

The Review component also re-uses formatting functions from TokenEnterAmount.tsx for better consistency.

P.S. Actual implementation (Review component + SendConfirmation changes) takes ~400 lines. Most of the other changes are necessary snapshot changes and new test file Review.test.tsx.
Also, Knip forced me to remove a component and move the function out of it to the different file which also trigger tiny changes in some extra unrelated files.

Test plan

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-01-09.at.14.02.46.mp4

Related issues

Relates to RET-1295

Backwards compatibility

Yes

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)

codecov · 2025-01-09T11:23:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (ee71b25) to head (a09bf64).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #6402    +/-   ##
========================================
  Coverage   89.10%   89.11%            
========================================
  Files         737      737            
  Lines       31881    31956    +75     
  Branches     5743     6060   +317     
========================================
+ Hits        28409    28478    +69     
- Misses       3424     3431     +7     
+ Partials       48       47     -1

Files with missing lines	Coverage Δ
src/account/utils.ts	`100.00% <100.00%> (ø)`
src/components/Avatar.tsx	`100.00% <100.00%> (ø)`
src/components/ContactCircle.tsx	`100.00% <100.00%> (ø)`
src/components/Review.tsx	`100.00% <100.00%> (ø)`
src/recipients/recipient.ts	`78.90% <100.00%> (ø)`
src/send/SendConfirmation.tsx	`96.55% <100.00%> (+0.71%)`	⬆️
src/tokens/hooks.ts	`98.03% <100.00%> (+0.42%)`	⬆️

... and 4 files 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 ee71b25...a09bf64. Read the comment docs.

…ed by Review.test.tsx

sviderock · 2025-01-09T12:16:59Z

src/components/ContactCircle.tsx

-    return <DefaultIcon foregroundColor={fontColor} backgroundColor={iconBackgroundColor} />
+    return (
+      <DefaultIcon
+        size={iconSize / 1.625}


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)

sviderock · 2025-01-09T12:21:03Z

src/send/SendConfirmation.test.tsx

-
-  it('renders address for phone recipients with multiple addresses', () => {
-    const screenProps = getMockStackScreenProps(Screens.SendConfirmation, {
-      transactionData: {
-        ...mockTokenTransactionData,
-        recipient: mockRecipient, // recipient that includes a PN
-      },
-      origin: SendOrigin.AppSendFlow,
-      isFromScan: false,
-    })
-    const { getByTestId } = renderScreen(
-      {
-        identity: {
-          e164NumberToAddress: {
-            [mockE164Number]: [mockAccount3, mockAccount2],
-          },
-        },
-      },
-      screenProps
-    )
-
-    expect(getByTestId('RecipientAddress')).toBeTruthy()
-  })
-
-  it.each([
-    { testSuffix: 'non phone number recipients', recipient: mockAddressRecipient },
-    { testSuffix: 'phone number recipient with one address', recipient: mockRecipient },
-  ])('does not render address for $testSuffix', ({ recipient }) => {
-    const screenProps = getMockStackScreenProps(Screens.SendConfirmation, {
-      transactionData: {
-        ...mockTokenTransactionData,
-        recipient,
-      },
-      origin: SendOrigin.AppSendFlow,
-      isFromScan: false,
-    })
-    const { queryByTestId } = renderScreen(
-      {
-        identity: {
-          e164NumberToAddress: {
-            [mockE164Number]: [mockAccount3],
-          },
-        },
-      },
-      screenProps
-    )
-
-    expect(queryByTestId('RecipientAddress')).toBeFalsy()
-  })


These seem to be redundant with the new way of showing the contact and considering that the contact view itself is tested in Review.test.tsx

sviderock · 2025-01-09T12:22:49Z

src/tokens/hooks.ts

+    const displayTokenAmount = getDisplayTokenAmount(tokenAmount, token)
+    const displayLocalAmount = getDisplayLocalAmount(tokenToLocal, localCurrencySymbol)


I think it might make sense to move getDisplayTokenAmount/getDisplayLocalAmount out of TokenEnterAmount.tsx if we're gonna use this across the board for the consistency?

sviderock · 2025-01-10T08:09:56Z

e2e/src/usecases/Send.js

@@ -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()


Addresses are now showed in full, not shortened

kathaypacific

sorry i haven't actually finished reviewing but need to go offline for a bit so i thought i'd submit the partial review for now in case the early feedback is helpful. i'll continue this afternoon.

one thought i had is that now the enter amount screens are all consistent, and they all prepare transactions (since we need to show the network fees). so we should pass the prepared transactions to the review screens, and there should be no need to prepare the transactions again and therefore no need to cater for a loading state right? i know the prepare transactions logic is existing but perhaps it's not needed anymore and can be simplified?

kathaypacific · 2025-01-10T08:43:51Z

src/components/ContactCircle.tsx

@@ -30,7 +34,7 @@ function ContactCircle({
  backgroundColor,
  foregroundColor,
  borderColor,
-  DefaultIcon = ({ foregroundColor }) => <User color={foregroundColor} />,
+  DefaultIcon = ({ color, size = 20 }) => <User size={size} color={color} />,


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

that's indeed redundant, will remove!

kathaypacific · 2025-01-10T09:50:18Z

src/tokens/hooks.ts

@@ -187,3 +194,32 @@ export function useAmountAsUsd(amount: BigNumber, tokenId: string | undefined) {
  }
  return amount.multipliedBy(tokenInfo.priceUsd)
 }
+
+export function useDisplayAmount({


can we please add a comment about what the expected resulting amount looks like? i think that there is a tendency for these generically named "toDisplayFormat" type of functions to become duplicated because it is not obvious what it is trying to do and when someone needs something similar, it's difficult to know that it already exists.

along this line of thinking, what do you think about accepting the local currency symbol and usd to local rate as props so that it can be a pure function rather than a hook? then we can start to have a collection of these formatting functions in one place, so that it is easier to find what you need?

Agree, that does make a lot of sense. One of my intentions with this specific function was to also memoize formatting of the string as it invokes 3 functions just to format that string (even though it's definitely not a performance bottleneck). This was more of a "if it takes basically no effort to reduce recalculations - I'll just do it" type of thing.

But I totally agree with the point of having it as a pure function and having an explanatory comment for better clarity. Will change the implementation!

kathaypacific · 2025-01-10T09:51:41Z

src/tokens/hooks.ts

@@ -187,3 +194,32 @@ export function useAmountAsUsd(amount: BigNumber, tokenId: string | undefined) {
  }
  return amount.multipliedBy(tokenInfo.priceUsd)
 }
+
+export function useDisplayAmount({


it'd also be great to add a test for this

kathaypacific · 2025-01-10T09:53:58Z

src/components/Review.tsx

+import { Spacing } from 'src/styles/styles'
+import variables from 'src/styles/variables'
+
+export function Review(props: {


Suggested change

export function Review(props: {

export function ReviewTransaction(props: {

kathaypacific · 2025-01-10T09:55:14Z

src/components/Review.tsx

+  title: string
+  children: ReactNode
+  headerAction?: ReactNode
+  isModal?: boolean


will this be used? i thought we were going to make any review modals into screens?

I thought that it might still be used (noticed usage of Screens.SendConfirmationModal from within bidali exchange flow?) but I'll double check it. There wasn't a particular screen in design where that particular modal changes (Screens.SendConfirmation was a separate screen from the start, not a modal) so I've kept it unchanged but I should've clarified this with Kayla. I'll talk to her about this one!

kathaypacific · 2025-01-10T09:56:45Z

src/components/Review.tsx

+      </Text>
+      <View style={styles.reviewSummaryItemContent}>
+        {props.icon}
+        <View style={{ flexShrink: 1 }}>


should use stylesheet instead?

I've noticed throughout the codebase that there are sometimes inline styles present and I assumed that this is fine for some occasions. It feels like sometimes (and specifically in this case) the style is so rudementary that it makes it clearer to understand what it does if it stays inline. That way it's clear that it's a "patch-like" kind of style. Otherwise, I'll need to provide a classname like "reviewSummaryItemContentTitlesWrapper` that doesn't provide any clarity or context on what is does (and even contextually the name doesn't explain what it does) and only consists of a single property that just fixes a certain quirk/aspect.

I do acknowledge the downside of inline styles being an object that re-creates on every re-render (therefore not keeping the same reference as it would be if it was created within the styles object) but my understanding is that's not a performance issue at all as of now, especially with the components that don't tend to re-render much.

i think any inline styles that are in the codebase that aren't actively using dynamic variables are unintentional...IMO consistency makes it worthwhile. i'd also argue that it's additional mental energy to think "is there only one style? if so, can i name it easily? if so, i'll put it in the stylesheet and if not, i'll put it inline" - it's also less scalable because it makes it harder to add new styles (would need to decide to add to the inline styles, or when it's worth moving to the stylesheet). the readability topic is interesting, i can see why seeing the style inline makes it easy to understand, however i think seeing one style in isolation doesn't necessarily give you the whole picture...

kathaypacific · 2025-01-10T09:58:05Z

src/components/Review.tsx

+    flexShrink: 1,
+  },
+  reviewSummaryItem: {
+    gap: 4,


Suggested change

gap: 4,

gap: Spacing.Tiny4,

kathaypacific · 2025-01-10T09:58:13Z

src/components/Review.tsx

+  },
+  reviewSummaryItemContent: {
+    flexDirection: 'row',
+    gap: 8,


should use Spacing

kathaypacific · 2025-01-10T09:58:36Z

src/components/Review.tsx

+  reviewDetailsItemLabel: {
+    flexDirection: 'row',
+    alignItems: 'center',
+    gap: 6,


please use one of the Spacing values

This one was specifically 6.5 in designs which we don't have a direct spacing value for so I've just kept it like this. I don't think 4/8 will make much bigger visual difference but I feel like I see it and it nagged me that it wasn't pixel-perfect 😄 (even though I made it 6 and not 6.5 as in design but I'm too used to web px units disregarding decimal points)

oh for cases like this where the designs are using variables outside of our predefined values, please tell Kayla to update the designs - we need to hold each other accountable to the design system :)

sviderock · 2025-01-10T11:32:23Z

@kathaypacific it is indeed helpful! My understanding was that it was necessary to reload the quote on review screen just to make sure that it is up to date with what actually is gonna happen as I've noticed multiple times that in those few seconds when you go from enter amount screen to review - it might have different fee amount as it fluctuates a lot.

I do understand that it looks inconsistent to have one data on the enter amount and then slightly different data on the review but at the same time it is also true if we show the same data on both enter amount and review but then show different data on the actual transaction details one it's completed. Does it make sense?

But I'm happy to remove that loading state in case this indeed is unnecessary and it sure can be simplified!

kathaypacific · 2025-01-10T14:37:29Z

@kathaypacific it is indeed helpful! My understanding was that it was necessary to reload the quote on review screen just to make sure that it is up to date with what actually is gonna happen as I've noticed multiple times that in those few seconds when you go from enter amount screen to review - it might have different fee amount as it fluctuates a lot.

I do understand that it looks inconsistent to have one data on the enter amount and then slightly different data on the review but at the same time it is also true if we show the same data on both enter amount and review but then show different data on the actual transaction details one it's completed. Does it make sense?

But I'm happy to remove that loading state in case this indeed is unnecessary and it sure can be simplified!

hum, can i leave this with you to decide with Kayla and Laura? i only started thinking about this because i think that the send flow used to start with an enter amount screen that didn't prepare the transactions / show the gas price. this made it a necessity to do it on the review screen. with the new enter amount components, we'll make the user wait once on the enter amount step, and then make them wait again on the next screen. removing any redundant wait time would be a nice UX improvement.

i hear the feedback that preparing transactions again would give the most recent gas estimate in case of fluctuations. if we re-prepared the transaction on the review screen, the edge case we prevent is a user enters an amount -> waits some time before proceeding to the next screen, during that time the gas price changes dramatically where re-calculating the gas value on the review screen may prevent the user from executing a transaction that costs more in gas than they want.

however, i think the edge case still remains - even if we re-prepare the transaction on the review screen, the user could still wait some time before pressing the confirm button. during this time, the gas price could have changed and we are once again in the above scenario. so some part of me thinks that if we really cared about this scenario of the gas price changing between the time of send flow start and the time of transaction execution, we could solve the problem more holistically (e.g. refresh the prepared transaction every block time, no matter which screen you are on)

i don't have a strong preference for the outcome of this topic and it's all good if we want to keep it like this but i wanted to raise it to make sure we have thought about it and are not carrying forward complexities that existed due to constraints which are no longer there by default :)

sviderock · 2025-01-10T14:43:04Z

@kathaypacific I totally agree and now lean more to your explanation! I'll check it out with Kayla and Laura just to make sure but I think we are gonna land on the same conclusion as you've just described. I'll keep you posted!

kathaypacific

yay great! i like the compound components approach, it's neat 💅🏼

kathaypacific · 2025-01-10T15:10:13Z

src/send/SendConfirmation.tsx

-        left={<BackButton eventName={SendEvents.send_confirm_back} />}
-      />
-      <DisconnectBanner />
-      <ReviewFrame


🥳 you might be able to remove ReviewFrame now too?

Oh yeah, I can remove it! I thought it would've been caught by Knip but probably the sole reason that its own test file is importing it tricks Knip into considering it "used"

kathaypacific · 2025-01-10T15:11:40Z

src/send/SendConfirmation.tsx

+            testID="SendConfirmationNetwork"
+            label={t('transactionDetails.network')}
+            value={fees.networkName}
+            isLoading={prepareTransactionLoading}


i think that you can always show the network name (no loader needed)?

Ah yeah, me inattentive 🤦

kathaypacific · 2025-01-10T15:14:45Z

src/send/SendConfirmation.tsx

+          <ReviewSummaryItem
+            testID="SendConfirmationToken"
+            header="Sending"
+            icon={<TokenIcon token={tokenInfo!} />}


can we avoid tokenInfo!? i'm not sure what we should do if there is no tokenInfo and it seems unlikely, but perhaps we can log an error (so at least we know about it) and render null instead of risking an app crash? or perhaps the components here already handle no tokenInfo gracefully and we just need to make the type optional?

Sure, I'll put proper guardrails

kathaypacific · 2025-01-10T15:21:42Z

src/components/Review.test.tsx

+
+    // "t('unknown')" returns "mockedTranslation:unknown" as per our mock
+    expect(tree.getByTestId('ContactItem/Unknown/Title')).toHaveTextContent('unknown')
+  })


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

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

kathaypacific · 2025-01-10T15:23:24Z

src/components/Review.test.tsx

+      <ReviewSummaryItemContact header="Contact" recipient={recipient} testID="ContactItem" />
+    )
+
+    // "t('unknown')" returns "mockedTranslation:unknown" as per our mock


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

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

kathaypacific · 2025-01-10T15:26:07Z

src/components/Review.tsx

+      return { title: recipient.address, icon: WalletIcon, testID: 'Address' }
+    }
+
+    return { title: t('unknown'), icon: UserIcon, testID: 'Unknown' }


this is probably carried over logic but i don't think it's necessary - the recipient should always have a phone number or address, so it should never reach this line. (although the main thing i wanted to call out is that "Unknown" is very unfriendly and we probably don't want this as a default, but then i realised we don't need a default at all)

sviderock added 3 commits January 9, 2025 11:33

add new review component and apply it to SendConfirmation

8038fd7

adjust tests for SendConfirmation

49392f7

update snapshots related to contact svgs size changes

9fff8bb

sviderock added 3 commits January 9, 2025 13:42

add tests to Review component

d3f146f

fix tests

6db6d68

remove unnecessary tests from SendConfirmation as it is already cover…

0b70162

…ed by Review.test.tsx

sviderock marked this pull request as ready for review January 9, 2025 12:04

sviderock added 2 commits January 9, 2025 14:05

remove unused component

1c60a2c

move formatShortenedAddress away from removed component

13be246

sviderock commented Jan 9, 2025

View reviewed changes

sviderock added 5 commits January 9, 2025 15:34

update e2e tests affected by the new Review changes

5853011

fix e2e issue

36cff0f

fix e2e again

84d9b84

fix e2e once again

4c441c2

Merge branch 'main' into slava/update-review-screens

6491bd9

sviderock commented Jan 10, 2025

View reviewed changes

kathaypacific reviewed Jan 10, 2025

View reviewed changes

try to fix e2e again

a09bf64

		const displayTokenAmount = getDisplayTokenAmount(tokenAmount, token)
		const displayLocalAmount = getDisplayLocalAmount(tokenToLocal, localCurrencySymbol)

	export function Review(props: {
	export function ReviewTransaction(props: {

feat: add new Review component and apply it to SendConfirmation #6402

Are you sure you want to change the base?

feat: add new Review component and apply it to SendConfirmation #6402

Conversation

sviderock commented Jan 9, 2025 • edited Loading

Test plan

Related issues

Backwards compatibility

Network scalability

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sviderock Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Jan 10, 2025 • edited Loading

kathaypacific commented Jan 10, 2025

sviderock commented Jan 10, 2025

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sviderock commented Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

sviderock Jan 9, 2025 •

edited

Loading

sviderock Jan 10, 2025 •

edited

Loading

sviderock commented Jan 10, 2025 •

edited

Loading