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: refresh swap flow progress indicator #7496

Closed
wants to merge 25 commits into from

Conversation

gnewfield
Copy link
Contributor

@gnewfield gnewfield commented Oct 19, 2023

Description

Create new swap flow progress indicator that increases the perceptual speed of swapping. This is accomplished by laying out the steps vertically, and clearly displaying their current status and expected behavior.

Main issue (design link included): https://linear.app/uniswap/issue/WEB-2452/[design]-faster-perceptual-swap
Sub-issues also included:

Screen capture

Before

Mobile Desktop
Mobile Before Desktop Before

After

Mobile Desktop
Mobile After Desktop After

Test plan

QA (ie manual testing)

Test various swap flows on desktop and mobile devices:

  • Classic swap, ETH input (no change)
  • Classic swap, ERC-20 input (token approval and permit approval, no change)
  • Classic swap, ERC-20 input (no token approval, no permit approval)
  • Classic swap, ERC-20 input (token approval, no permit approval)
  • Classic swap, ERC-20 input (no token approval, permit approval)
  • Classic swap, USDT input (already approved and permitted but limit too low; revocation required for reset)
  • UniswapX order, ETH input (wrapping required)
  • UniswapX order, ERC-20 input

Devices

Desktop and mobile web

Automated testing

  • Unit test
  • Integration/E2E test

@linear
Copy link

linear bot commented Oct 19, 2023

WEB-2764 [swaptober] progress indicator v2

Implement WEB-2452

Time-To-Swap success metric: https://app.amplitude.com/analytics/uniswap/chart/0z25ec81/edit/jitgde31

10/5 value: 110 - 115 seconds on Ethereum mainnet

@vercel
Copy link

vercel bot commented Oct 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
interface ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 6, 2023 10:01pm

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #7496 (0a52b30) into main (443a00a) will increase coverage by 0.08%.
Report is 4 commits behind head on main.
The diff coverage is 53.22%.

Flag Coverage Δ
cloud-tests 83.60% <ø> (ø)
unit-tests 43.24% <53.22%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@cypress
Copy link

cypress bot commented Oct 19, 2023

17 failed and 1 flaky tests on run #15361 ↗︎

17 118 0 0 Flakiness 1

Details:

feat: refresh swap flow progress indicator
Project: Uniswap Interface Commit: 0a52b30b4c
Status: Failed Duration: 12:33 💡
Started: Nov 6, 2023 10:00 PM Ended: Nov 6, 2023 10:13 PM
Failed  swap/progressIndicator.test.ts • 9 failed tests • e2e

View Output Video

Test Artifacts
Progress Indicator > swaps when user has already approved token and permit2 Output Screenshots
Progress Indicator > swaps after handling user rejection of both approval and signature Output Screenshots
Progress Indicator > prompts token approval when existing approval amount is too low Output Screenshots
Progress Indicator > prompts signature when existing permit approval is expired Output Screenshots
Progress Indicator > prompts signature when existing permit approval amount is too low Output Screenshots
Progress Indicator > approval process > swaps after completing full permit2 approval process Output Screenshots
Progress Indicator > approval process > swaps with existing permit approval and missing token approval Output Screenshots
Progress Indicator > approval process > swaps USDT with existing permit, and existing but insufficient token approval Output Screenshots
Progress Indicator > approval process > swaps USDT with existing permit, and existing and sufficient token approval Output Screenshots
Failed  permit2.test.ts • 8 failed tests • e2e

View Output Video

Test Artifacts
Permit2 > swaps when user has already approved token and permit2 Output Screenshots
Permit2 > swaps after handling user rejection of both approval and signature Output Screenshots
Permit2 > prompts signature when existing permit approval is expired Output Screenshots
Permit2 > prompts signature when existing permit approval amount is too low Output Screenshots
Permit2 > approval process (with intermediate screens) > swaps after completing full permit2 approval process Output Screenshots
Permit2 > approval process (with intermediate screens) > swaps with existing permit approval and missing token approval Output Screenshots
Permit2 > approval process (with intermediate screens) > swaps USDT with existing permit, and existing but insufficient token approval Output Screenshots
Permit2 > approval process (with intermediate screens) > swaps USDT with existing permit, and existing and sufficient token approval Output Screenshots
Flakiness  cypress/e2e/swap/logging.test.ts • 1 flaky test • e2e

View Output Video

Test Artifacts
swap flow logging > completes two swaps and verifies the TTS logging for the first, plus all intermediate steps along the way Output Screenshots

Review all test suite changes for PR #7496 ↗︎

@@ -288,6 +294,7 @@ export default function ConfirmSwapModal({
fiatValueOutput: { data?: number; isLoading: boolean }
}) {
const { chainId } = useWeb3React()
const inputTokenColor = useColor(trade?.inputAmount.currency.wrapped)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a very nice touch, looks great in the animation

[ConfirmModalState.WRAPPING]: {
icon: <CurrencyLogo currency={trade?.inputAmount.currency} />,
rippleColor: inputTokenColor,
previewTitle: `Wrap ${nativeCurrency.symbol}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

all of the titles and link texts should be wrapped in Trans components and be passed as ReactElements to support i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for calling out. Tagged for translation using the t macro.

previewTitle: `Wrap ${nativeCurrency.symbol}`,
actionRequiredTitle: `Wrap ${nativeCurrency.symbol} in wallet`,
inProgressTitle: `Wrapping ${nativeCurrency.symbol}...`,
timeToEnd: chainId === ChainId.MAINNET ? 20 : 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

this is the same value every time it's used, should this just be part of the Step component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing a timeToEnd attribute implicitly indicates that a timer should be shown, which is not true for all steps, e.g. permit signature. So even if we move this value into the step component, we still need to pass an attribute here; it would just be boolean instead.

Also, as requested by design, I've made this value dynamic (displays actual estimate of transaction confirmation time), so it should be set by the ProgressIndicator, where the logic exists for computing it.

},
[ConfirmModalState.PERMITTING]: {
icon: <Sign />,
rippleColor: '#FC72FF',
Copy link
Contributor

Choose a reason for hiding this comment

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

should these ripple colors update darkMode vs lightMode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The icons for these steps remain the same, regardless of light and dark mode, so rippleColor is constant to always match the icon's fill.

actionRequiredTitle: `Approve in wallet`,
inProgressTitle: 'Approval pending...',
timeToEnd: chainId === ChainId.MAINNET ? 20 : 10,
delayedTitle: 'Longer than expected...',
Copy link
Contributor

Choose a reason for hiding this comment

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

On the approval step, the Longer than expected text never appears and you get a blank row
Screenshot 2023-11-01 at 10 13 01 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, small fix

opacity: 0.3;
}
`
const Ring = styled.div<{ width: string; color: string; animation: Keyframes }>`
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
const Ring = styled.div<{ width: string; color: string; animation: Keyframes }>`
const Ring = styled.div<{ $borderWidth: string; $borderColor: string; $animation: Keyframes }>`

Comment on lines 116 to 120
return isTimeRemaining ? (
<ThemedText.BodyPrimary>{stepDetails.actionRequiredTitle}</ThemedText.BodyPrimary>
) : (
<ThemedText.BodyPrimary>{stepDetails.delayedStartTitle}</ThemedText.BodyPrimary>
)
Copy link
Contributor

Choose a reason for hiding this comment

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

the component never changes, you can just change the content, same with the in_progress step

Suggested change
return isTimeRemaining ? (
<ThemedText.BodyPrimary>{stepDetails.actionRequiredTitle}</ThemedText.BodyPrimary>
) : (
<ThemedText.BodyPrimary>{stepDetails.delayedStartTitle}</ThemedText.BodyPrimary>
)
return (
<ThemedText.BodyPrimary>
{isTimeRemaining ? stepDetails.actionRequiredTitle : stepDetails.delayedStartTitle}
</ThemedText.BodyPrimary>
)

Comment on lines +137 to +138
const minutesText = minutes < 10 ? `0${minutes}` : minutes
const secondsText = seconds < 10 ? `0${seconds}` : seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

translate

export function Step({ stepStatus, stepDetails }: { stepStatus: StepStatus; stepDetails: StepDetails }) {
// Timer is shown in three cases:
// (1) User has a specified amount of time to perform a required action. Timer starts running as soon as the step becomes active.
// (2) Step has an estimated amount of time in which it should be completed. Timer is set but not running when step becomes active.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should still hide the timer until the step is in progress, it seems strange to me to have the timer in view and paused while waiting for user input

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 was thinking of it like a shot clock in sports; always remains on but doesn't start until activity begins. I agree with you though that it's simpler to just hide the timer until it's running. I've made the changes and also simplified the time-keeping logic.

const isSingleStep = confirmModalState !== ConfirmModalState.REVIEWING && pendingModalSteps.length === 1
if (showProgressIndicatorV2 && (isSingleStep || lastStepComplete)) {
return null
} else if (!showProgressIndicatorV2 && confirmModalState !== ConfirmModalState.REVIEWING && !showAcceptChanges) {
return null
}
return <SwapModalHeader inputCurrency={inputCurrency} trade={trade} allowedSlippage={allowedSlippage} />
Copy link
Contributor

Choose a reason for hiding this comment

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

header X is missing clickable style
Screenshot 2023-11-01 at 10 18 12 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ClickableStyle to the shared CloseIcon component

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