From 2ad9aaaeaba3a17e2a8415b66ad4ade5126db241 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Wed, 16 Oct 2024 17:05:55 +0100 Subject: [PATCH 1/6] fix: remove scroll to bottom when confirmation is transaction redesigned --- .../scroll-to-bottom/scroll-to-bottom.test.tsx | 15 ++++++++++++++- .../confirm/scroll-to-bottom/scroll-to-bottom.tsx | 11 ++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.test.tsx b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.test.tsx index d5954e56609b..6bf31166b6e5 100644 --- a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.test.tsx +++ b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.test.tsx @@ -2,7 +2,10 @@ import React from 'react'; import configureMockStore from 'redux-mock-store'; import { unapprovedTypedSignMsgV4 } from '../../../../../../test/data/confirmations/typed_sign'; -import { getMockPersonalSignConfirmState } from '../../../../../../test/data/confirmations/helper'; +import { + getMockContractInteractionConfirmState, + getMockPersonalSignConfirmState, +} from '../../../../../../test/data/confirmations/helper'; import { renderWithConfirmContextProvider } from '../../../../../../test/lib/confirmations/render-helpers'; import * as usePreviousHooks from '../../../../../hooks/usePrevious'; import ScrollToBottom from './scroll-to-bottom'; @@ -116,6 +119,16 @@ describe('ScrollToBottom', () => { expect(mockSetHasScrolledToBottom).toHaveBeenCalledWith(false); }); + it('does not render the scroll button when the confirmation is transaction redesigned', () => { + const mockStateTransaction = getMockContractInteractionConfirmState(); + const { container } = renderWithConfirmContextProvider( + foobar, + configureMockStore([])(mockStateTransaction), + ); + + expect(container.querySelector(buttonSelector)).not.toBeInTheDocument(); + }); + describe('when user has scrolled to the bottom', () => { beforeEach(() => { mockedUseScrollRequiredResult.isScrolledToBottom = true; diff --git a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx index f42997de114e..f09b18bec0c1 100644 --- a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx +++ b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx @@ -1,5 +1,6 @@ import React, { useContext, useEffect } from 'react'; import { useSelector } from 'react-redux'; +import { TransactionType } from '@metamask/transaction-controller'; import { Box, ButtonIcon, @@ -20,6 +21,7 @@ import { usePrevious } from '../../../../../hooks/usePrevious'; import { useScrollRequired } from '../../../../../hooks/useScrollRequired'; import { useConfirmContext } from '../../../context/confirm'; import { selectConfirmationAdvancedDetailsOpen } from '../../../selectors/preferences'; +import { REDESIGN_USER_TRANSACTION_TYPES } from '../../../utils'; type ContentProps = { /** @@ -49,6 +51,13 @@ const ScrollToBottom = ({ children }: ContentProps) => { offsetPxFromBottom: 0, }); + const isTransaction = REDESIGN_USER_TRANSACTION_TYPES.includes( + currentConfirmation?.type as TransactionType, + ); + + const showScrollToBottom = + isScrollable && !isScrolledToBottom && !isTransaction; + /** * Scroll to the top of the page when the confirmation changes. This happens * when we navigate through different confirmations. Also, resets hasScrolledToBottom @@ -104,7 +113,7 @@ const ScrollToBottom = ({ children }: ContentProps) => { > {children} - {isScrollable && !isScrolledToBottom && ( + {showScrollToBottom && ( Date: Thu, 17 Oct 2024 06:10:22 +0100 Subject: [PATCH 2/6] fix: e2e for transaction redesign --- test/e2e/tests/confirmations/helpers.ts | 4 ++++ .../transactions/erc721-approve-redesign.spec.ts | 6 +++--- .../increase-token-allowance-redesign.spec.ts | 4 ++-- .../transactions/revoke-allowance-redesign.spec.ts | 6 +++--- test/e2e/tests/confirmations/transactions/shared.ts | 7 +++++-- .../confirm/scroll-to-bottom/scroll-to-bottom.tsx | 9 +++++++-- 6 files changed, 24 insertions(+), 12 deletions(-) diff --git a/test/e2e/tests/confirmations/helpers.ts b/test/e2e/tests/confirmations/helpers.ts index ff467f42c320..a61cbc0b5011 100644 --- a/test/e2e/tests/confirmations/helpers.ts +++ b/test/e2e/tests/confirmations/helpers.ts @@ -14,6 +14,10 @@ export async function scrollAndConfirmAndAssertConfirm(driver: Driver) { await driver.clickElement('[data-testid="confirm-footer-button"]'); } +export async function confirmAndAssertConfirm(driver: Driver) { + await driver.clickElement('[data-testid="confirm-footer-button"]'); +} + export function withRedesignConfirmationFixtures( // Default params first is discouraged because it makes it hard to call the function without the // optional parameters. But it doesn't apply here because we're always passing in a variable for diff --git a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts index c7ceb6c42c94..7f9497ca8b5f 100644 --- a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts @@ -2,7 +2,7 @@ import { MockttpServer } from 'mockttp'; import { WINDOW_TITLES } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; -import { scrollAndConfirmAndAssertConfirm } from '../helpers'; +import { confirmAndAssertConfirm } from '../helpers'; import { openDAppWithContract, TestSuiteArguments, @@ -126,7 +126,7 @@ export async function confirmMintTransaction(driver: Driver) { text: 'Transaction request', }); - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); // Verify Mint Transaction is Confirmed before proceeding await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); @@ -192,7 +192,7 @@ async function assertApproveDetails(driver: Driver) { } async function confirmApproveTransaction(driver: Driver) { - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); await driver.waitUntilXWindowHandles(2); await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); diff --git a/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts index 4eed23b20f44..0b6334eb698d 100644 --- a/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts @@ -11,7 +11,7 @@ import { Mockttp } from '../../../mock-e2e'; import GanacheContractAddressRegistry from '../../../seeder/ganache-contract-address-registry'; import { SMART_CONTRACTS } from '../../../seeder/smart-contracts'; import { Driver } from '../../../webdriver/driver'; -import { scrollAndConfirmAndAssertConfirm } from '../helpers'; +import { confirmAndAssertConfirm } from '../helpers'; import { openDAppWithContract, TestSuiteArguments } from './shared'; describe('Confirmation Redesign ERC20 Increase Allowance', function () { @@ -119,7 +119,7 @@ async function createAndAssertIncreaseAllowanceSubmission( await editSpendingCap(driver, newSpendingCap); - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, newSpendingCap); } diff --git a/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts index ba97d9cda4cd..6f6212ef061c 100644 --- a/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts @@ -2,7 +2,7 @@ import { MockttpServer } from 'mockttp'; import { WINDOW_TITLES } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; -import { scrollAndConfirmAndAssertConfirm } from '../helpers'; +import { confirmAndAssertConfirm } from '../helpers'; import { mocked4BytesApprove } from './erc20-approve-redesign.spec'; import { assertChangedSpendingCap, @@ -53,7 +53,7 @@ describe('Confirmation Redesign ERC20 Revoke Allowance', function () { text: 'Remove permission', }); - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, NEW_SPENDING_CAP); }, @@ -91,7 +91,7 @@ describe('Confirmation Redesign ERC20 Revoke Allowance', function () { text: 'Remove permission', }); - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, NEW_SPENDING_CAP); }, diff --git a/test/e2e/tests/confirmations/transactions/shared.ts b/test/e2e/tests/confirmations/transactions/shared.ts index 62ceb318aa48..2d5c7f7b0010 100644 --- a/test/e2e/tests/confirmations/transactions/shared.ts +++ b/test/e2e/tests/confirmations/transactions/shared.ts @@ -10,7 +10,10 @@ const { openDapp, WINDOW_TITLES, } = require('../../../helpers'); -const { scrollAndConfirmAndAssertConfirm } = require('../helpers'); +const { + scrollAndConfirmAndAssertConfirm, + confirmAndAssertConfirm, +} = require('../helpers'); export type TestSuiteArguments = { driver: Driver; @@ -111,7 +114,7 @@ export async function confirmDepositTransaction(driver: Driver) { }); await driver.delay(veryLargeDelayMs); - await scrollAndConfirmAndAssertConfirm(driver); + await confirmAndAssertConfirm(driver); } export async function confirmDepositTransactionWithCustomNonce( diff --git a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx index f09b18bec0c1..a11a552b02fd 100644 --- a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx +++ b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx @@ -51,12 +51,12 @@ const ScrollToBottom = ({ children }: ContentProps) => { offsetPxFromBottom: 0, }); - const isTransaction = REDESIGN_USER_TRANSACTION_TYPES.includes( + const isTransactionRedesign = REDESIGN_USER_TRANSACTION_TYPES.includes( currentConfirmation?.type as TransactionType, ); const showScrollToBottom = - isScrollable && !isScrolledToBottom && !isTransaction; + isScrollable && !isScrolledToBottom && !isTransactionRedesign; /** * Scroll to the top of the page when the confirmation changes. This happens @@ -80,6 +80,11 @@ const ScrollToBottom = ({ children }: ContentProps) => { }, [currentConfirmation?.id, previousId, ref?.current]); useEffect(() => { + if (isTransactionRedesign) { + setIsScrollToBottomCompleted(true); + return; + } + setIsScrollToBottomCompleted(!isScrollable || hasScrolledToBottom); }, [isScrollable, hasScrolledToBottom]); From 75bbb9e4ed96b2dcfd448a32cb08f537ac47a98a Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Thu, 17 Oct 2024 13:20:26 +0100 Subject: [PATCH 3/6] fix: add missing dependencies --- .../components/confirm/scroll-to-bottom/scroll-to-bottom.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx index a11a552b02fd..9ea723414fa0 100644 --- a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx +++ b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx @@ -86,7 +86,7 @@ const ScrollToBottom = ({ children }: ContentProps) => { } setIsScrollToBottomCompleted(!isScrollable || hasScrolledToBottom); - }, [isScrollable, hasScrolledToBottom]); + }, [isScrollable, hasScrolledToBottom, isTransactionRedesign]); return ( Date: Sun, 20 Oct 2024 14:15:25 +0100 Subject: [PATCH 4/6] apply suggestions --- .../components/confirm/scroll-to-bottom/scroll-to-bottom.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx index 9ea723414fa0..c61053818923 100644 --- a/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx +++ b/ui/pages/confirmations/components/confirm/scroll-to-bottom/scroll-to-bottom.tsx @@ -21,7 +21,7 @@ import { usePrevious } from '../../../../../hooks/usePrevious'; import { useScrollRequired } from '../../../../../hooks/useScrollRequired'; import { useConfirmContext } from '../../../context/confirm'; import { selectConfirmationAdvancedDetailsOpen } from '../../../selectors/preferences'; -import { REDESIGN_USER_TRANSACTION_TYPES } from '../../../utils'; +import { REDESIGN_DEV_TRANSACTION_TYPES } from '../../../utils'; type ContentProps = { /** @@ -51,7 +51,7 @@ const ScrollToBottom = ({ children }: ContentProps) => { offsetPxFromBottom: 0, }); - const isTransactionRedesign = REDESIGN_USER_TRANSACTION_TYPES.includes( + const isTransactionRedesign = REDESIGN_DEV_TRANSACTION_TYPES.includes( currentConfirmation?.type as TransactionType, ); From bdf56de4291324294627823cebc85ecc7e960ae9 Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Tue, 29 Oct 2024 10:47:05 +0000 Subject: [PATCH 5/6] revert e2e changes --- test/e2e/tests/confirmations/helpers.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/e2e/tests/confirmations/helpers.ts b/test/e2e/tests/confirmations/helpers.ts index a61cbc0b5011..9954a91dd758 100644 --- a/test/e2e/tests/confirmations/helpers.ts +++ b/test/e2e/tests/confirmations/helpers.ts @@ -15,6 +15,7 @@ export async function scrollAndConfirmAndAssertConfirm(driver: Driver) { } export async function confirmAndAssertConfirm(driver: Driver) { + await driver.clickElementSafe('.confirm-scroll-to-bottom__button'); await driver.clickElement('[data-testid="confirm-footer-button"]'); } From 987c491d1640a1ac7a2b1b9431f0a2a48e2534fd Mon Sep 17 00:00:00 2001 From: Vinicius Stevam Date: Wed, 30 Oct 2024 13:55:56 +0000 Subject: [PATCH 6/6] revert e2e test --- test/e2e/tests/confirmations/helpers.ts | 5 ----- .../transactions/erc721-approve-redesign.spec.ts | 6 +++--- .../transactions/increase-token-allowance-redesign.spec.ts | 4 ++-- .../transactions/revoke-allowance-redesign.spec.ts | 6 +++--- test/e2e/tests/confirmations/transactions/shared.ts | 7 ++----- 5 files changed, 10 insertions(+), 18 deletions(-) diff --git a/test/e2e/tests/confirmations/helpers.ts b/test/e2e/tests/confirmations/helpers.ts index 9954a91dd758..ff467f42c320 100644 --- a/test/e2e/tests/confirmations/helpers.ts +++ b/test/e2e/tests/confirmations/helpers.ts @@ -14,11 +14,6 @@ export async function scrollAndConfirmAndAssertConfirm(driver: Driver) { await driver.clickElement('[data-testid="confirm-footer-button"]'); } -export async function confirmAndAssertConfirm(driver: Driver) { - await driver.clickElementSafe('.confirm-scroll-to-bottom__button'); - await driver.clickElement('[data-testid="confirm-footer-button"]'); -} - export function withRedesignConfirmationFixtures( // Default params first is discouraged because it makes it hard to call the function without the // optional parameters. But it doesn't apply here because we're always passing in a variable for diff --git a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts index 7f9497ca8b5f..c7ceb6c42c94 100644 --- a/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/erc721-approve-redesign.spec.ts @@ -2,7 +2,7 @@ import { MockttpServer } from 'mockttp'; import { WINDOW_TITLES } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; -import { confirmAndAssertConfirm } from '../helpers'; +import { scrollAndConfirmAndAssertConfirm } from '../helpers'; import { openDAppWithContract, TestSuiteArguments, @@ -126,7 +126,7 @@ export async function confirmMintTransaction(driver: Driver) { text: 'Transaction request', }); - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); // Verify Mint Transaction is Confirmed before proceeding await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); @@ -192,7 +192,7 @@ async function assertApproveDetails(driver: Driver) { } async function confirmApproveTransaction(driver: Driver) { - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); await driver.waitUntilXWindowHandles(2); await driver.switchToWindowWithTitle(WINDOW_TITLES.ExtensionInFullScreenView); diff --git a/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts index 0b6334eb698d..4eed23b20f44 100644 --- a/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/increase-token-allowance-redesign.spec.ts @@ -11,7 +11,7 @@ import { Mockttp } from '../../../mock-e2e'; import GanacheContractAddressRegistry from '../../../seeder/ganache-contract-address-registry'; import { SMART_CONTRACTS } from '../../../seeder/smart-contracts'; import { Driver } from '../../../webdriver/driver'; -import { confirmAndAssertConfirm } from '../helpers'; +import { scrollAndConfirmAndAssertConfirm } from '../helpers'; import { openDAppWithContract, TestSuiteArguments } from './shared'; describe('Confirmation Redesign ERC20 Increase Allowance', function () { @@ -119,7 +119,7 @@ async function createAndAssertIncreaseAllowanceSubmission( await editSpendingCap(driver, newSpendingCap); - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, newSpendingCap); } diff --git a/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts b/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts index 6f6212ef061c..ba97d9cda4cd 100644 --- a/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts +++ b/test/e2e/tests/confirmations/transactions/revoke-allowance-redesign.spec.ts @@ -2,7 +2,7 @@ import { MockttpServer } from 'mockttp'; import { WINDOW_TITLES } from '../../../helpers'; import { Driver } from '../../../webdriver/driver'; -import { confirmAndAssertConfirm } from '../helpers'; +import { scrollAndConfirmAndAssertConfirm } from '../helpers'; import { mocked4BytesApprove } from './erc20-approve-redesign.spec'; import { assertChangedSpendingCap, @@ -53,7 +53,7 @@ describe('Confirmation Redesign ERC20 Revoke Allowance', function () { text: 'Remove permission', }); - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, NEW_SPENDING_CAP); }, @@ -91,7 +91,7 @@ describe('Confirmation Redesign ERC20 Revoke Allowance', function () { text: 'Remove permission', }); - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); await assertChangedSpendingCap(driver, NEW_SPENDING_CAP); }, diff --git a/test/e2e/tests/confirmations/transactions/shared.ts b/test/e2e/tests/confirmations/transactions/shared.ts index 2d5c7f7b0010..62ceb318aa48 100644 --- a/test/e2e/tests/confirmations/transactions/shared.ts +++ b/test/e2e/tests/confirmations/transactions/shared.ts @@ -10,10 +10,7 @@ const { openDapp, WINDOW_TITLES, } = require('../../../helpers'); -const { - scrollAndConfirmAndAssertConfirm, - confirmAndAssertConfirm, -} = require('../helpers'); +const { scrollAndConfirmAndAssertConfirm } = require('../helpers'); export type TestSuiteArguments = { driver: Driver; @@ -114,7 +111,7 @@ export async function confirmDepositTransaction(driver: Driver) { }); await driver.delay(veryLargeDelayMs); - await confirmAndAssertConfirm(driver); + await scrollAndConfirmAndAssertConfirm(driver); } export async function confirmDepositTransactionWithCustomNonce(