wallet icon indicating copy to clipboard operation
wallet copied to clipboard

feat(swap): Initial Swap screen redesign

Open jophish opened this issue 9 months ago • 3 comments

Description

First part of RET-1065. Updates the design of swap screen to match the designs in Figma. The rest of the behavior outlined in the ticket will come in followup PRs.

Test plan

Unit and manual tested. See video below:

https://github.com/valora-inc/wallet/assets/569401/c078eb34-4cd2-4fe9-a9a2-f5eba85c0c47

Related issues

Backwards compatibility

Yes.

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)

jophish avatar May 13 '24 21:05 jophish

Codecov Report

Attention: Patch coverage is 89.47368% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 86.31%. Comparing base (5946f19) to head (6851300).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5409   +/-   ##
=======================================
  Coverage   86.31%   86.31%           
=======================================
  Files         756      756           
  Lines       31185    31205   +20     
  Branches     5347     5353    +6     
=======================================
+ Hits        26916    26935   +19     
- Misses       4036     4037    +1     
  Partials      233      233           
Files Coverage Δ
src/analytics/Events.tsx 100.00% <100.00%> (ø)
src/analytics/Properties.tsx 100.00% <ø> (ø)
src/components/Touchable.tsx 100.00% <100.00%> (ø)
src/icons/DownIndicator.tsx 100.00% <100.00%> (ø)
src/swap/SwapScreen.tsx 93.78% <100.00%> (+0.09%) :arrow_up:
src/swap/SwapAmountInput.tsx 91.66% <83.78%> (+0.62%) :arrow_up:

... 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 5946f19...6851300. Read the comment docs.

codecov[bot] avatar May 13 '24 21:05 codecov[bot]

Looking great!

Noticed a few rendering issues:

  1. The switch token button is rendered behind the "to" field on the iOS emulator (see screenshot):

  2. If we put a really long number, everything breaks (see video). This could possibly be a real-life issue for fiats having really large denominations. Perhaps we could stop the fiat value from growing in width past 40% of the parent width?

https://github.com/valora-inc/wallet/assets/2737872/bd25e8a4-2cff-4039-a72a-82d4f4e83902

  1. +1 to Jean:
  • I would expect the whole top part of the swap amount box to be pressable. Not only the arrow button.
  • I wonder if instead of ripple across the switch token button, we can add a rotation animation instead? Like, the arrow after a tap rotates 360 degrees (don't know though if it is possible to turn off the ripple effect).

bakoushin avatar May 14 '24 09:05 bakoushin

Responding to some comments:

@jeanregisser

Does the token selection also work when tapping the container of the down arrow?

It did not, but now it does - this required a bit of rejiggering to how Touchables work, since the border radius is uneven depending on the state of the screen.

The right padding/margin in the skeleton loader seems different from the left one. It should be the same.

Thanks! This has been fixed.

The ripple effect on the switch token button is being clipped by containers above and below it. It should be above them.

This was a tricky one. I'm somewhat sure that we were running up against this bug, since I did not encounter the adverse overlapping behavior when using PlatformTouchable.SelectableBackground() as the background. Fortunately, I was able to work around this using the foreground option instead of background as described here.

I'm slightly concerned by the fact that the "MAX" button can change position based on the length of the $ value. Maybe something we can improve with Kayla?

The designs have been updated to move the "MAX" button to the right of the fiat estimation.

@bakoushin

The switch token button is rendered behind the "to" field on the iOS emulator (see screenshot):

Would you mind testing on iOS again? This is working on Android, but made a few small changes.

If we put a really long number, everything breaks (see video). This could possibly be a real-life issue for fiats having really large denominations.

I've raised this with Kayla, waiting to hear back about design ideas.

I would expect the whole top part of the swap amount box to be pressable. Not only the arrow button.

Addressed above!

I wonder if instead of ripple across the switch token button, we can add a rotation animation instead?

I suppose we could do this, though if the ripple is working correctly (which it should be, now) we could also stick with that.

See new video here:

https://github.com/valora-inc/wallet/assets/569401/3ee616bc-937d-43d9-87eb-cebbf73a5255

jophish avatar May 14 '24 20:05 jophish