Skip to content

Commit 5f8f6b5

Browse files
authored
fix: Revert "feat(14507): improve error message for failed txn in activity… (#31137)
… details view (#29338)" This reverts commit d510d5c. > ### NOTE > [transaction-status-label.test.js](https://github.com/MetaMask/metamask-extension/blob/ba134eed8a231112008116aec1bc1e40ddf9e808/ui/components/app/transaction-status-label/transaction-status-label.test.js) was full of conflicts. I rewrote the tests based on what I could piece together. <!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** This change was intentional, but it turned out to be incorrect. @DDDDDanica based her PR on this ticket linked below, which refers to a Figma file. However, the missing context is that the Figma file included designs specifically for Smart Transactions. [[UX Improvement]: Transaction Failure Message Not Easily Found #14507](#14507): > Expected final result > https://www.figma.com/design/ZzVQ6iu13C67K807Z1bg5I/Smart-Transactions-1.0?node-id=4296-25303&t=XN3hVQrE3oWM0lCA-1 <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31137?quickstart=1) ## **Related issues** * Related: MetaMask/MetaMask-planning#4198 * Related: #14507 ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
1 parent 9a485df commit 5f8f6b5

24 files changed

+159
-263
lines changed

app/_locales/de/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/el/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/en/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/en_GB/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/es/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/fr/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/hi/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/id/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/ja/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/ko/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/pt/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/ru/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/tl/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/tr/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/vi/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

app/_locales/zh_CN/messages.json

-3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

ui/components/app/transaction-list-item-details/index.scss

+2-5
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,6 @@
4949
display: flex;
5050
flex-direction: column;
5151
align-items: flex-end;
52-
height: 42px;
53-
justify-content: space-between;
5452

5553
.btn-link {
5654
font-size: 12px;
@@ -64,10 +62,9 @@
6462
}
6563

6664
&__operations {
67-
margin: 0 16px 16px 16px;
65+
margin: 0 0 16px 16px;
6866
display: flex;
69-
justify-content: space-between;
70-
align-items: center;
67+
justify-content: flex-end;
7168
}
7269

7370
&__header {

ui/components/app/transaction-list-item-details/transaction-list-item-details.component.js

+15-30
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,6 @@ import Tooltip from '../../ui/tooltip';
1313
import CancelButton from '../cancel-button';
1414
import Popover from '../../ui/popover';
1515
import { Box } from '../../component-library/box';
16-
import { Text } from '../../component-library/text';
17-
import {
18-
BannerAlert,
19-
BannerAlertSeverity,
20-
} from '../../component-library/banner-alert';
21-
import { TextVariant } from '../../../helpers/constants/design-system';
2216
import { SECOND } from '../../../../shared/constants/time';
2317
import { MetaMetricsEventCategory } from '../../../../shared/constants/metametrics';
2418
import { getURLHostName } from '../../../helpers/utils/util';
@@ -52,7 +46,6 @@ export default class TransactionListItemDetails extends PureComponent {
5246
senderNickname: PropTypes.string.isRequired,
5347
transactionStatus: PropTypes.func,
5448
isCustomNetwork: PropTypes.bool,
55-
showErrorBanner: PropTypes.bool,
5649
history: PropTypes.object,
5750
blockExplorerLinkText: PropTypes.object,
5851
chainId: PropTypes.string,
@@ -163,7 +156,6 @@ export default class TransactionListItemDetails extends PureComponent {
163156
title,
164157
onClose,
165158
showCancel,
166-
showErrorBanner,
167159
transactionStatus: TransactionStatus,
168160
blockExplorerLinkText,
169161
} = this.props;
@@ -177,17 +169,6 @@ export default class TransactionListItemDetails extends PureComponent {
177169
<Popover title={title} onClose={onClose}>
178170
<div className="transaction-list-item-details">
179171
<div className="transaction-list-item-details__operations">
180-
{showErrorBanner && (
181-
<BannerAlert severity={BannerAlertSeverity.Warning}>
182-
<Text
183-
variant={TextVariant.bodyMd}
184-
as="h6"
185-
data-testid="transaction-list-item-details-banner-error-message"
186-
>
187-
{t('transactionFailedBannerMessage')}
188-
</Text>
189-
</BannerAlert>
190-
)}
191172
<div className="transaction-list-item-details__header-buttons">
192173
{showSpeedUp && (
193174
<Button
@@ -214,7 +195,7 @@ export default class TransactionListItemDetails extends PureComponent {
214195
className="transaction-list-item-details__header-button"
215196
data-testid="rety-button"
216197
>
217-
<i className="fa fa-sync" />
198+
<i className="fa fa-sync"></i>
218199
</Button>
219200
</Tooltip>
220201
)}
@@ -226,18 +207,22 @@ export default class TransactionListItemDetails extends PureComponent {
226207
data-testid="transaction-list-item-details-tx-status"
227208
>
228209
<div>{t('status')}</div>
229-
<TransactionStatus />
210+
<div>
211+
<TransactionStatus />
212+
</div>
230213
</div>
231214
<div className="transaction-list-item-details__tx-hash">
232-
<Button
233-
type="link"
234-
onClick={this.handleBlockExplorerClick}
235-
disabled={!hash}
236-
>
237-
{blockExplorerLinkText.firstPart === 'addBlockExplorer'
238-
? t('addBlockExplorer')
239-
: t('viewOnBlockExplorer')}
240-
</Button>
215+
<div>
216+
<Button
217+
type="link"
218+
onClick={this.handleBlockExplorerClick}
219+
disabled={!hash}
220+
>
221+
{blockExplorerLinkText.firstPart === 'addBlockExplorer'
222+
? t('addBlockExplorer')
223+
: t('viewOnBlockExplorer')}
224+
</Button>
225+
</div>
241226
<div>
242227
<Tooltip
243228
wrapperClassName="transaction-list-item-details__header-button"

ui/components/app/transaction-list-item-details/transaction-list-item-details.component.test.js

+28-25
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import React from 'react';
22
import configureMockStore from 'redux-mock-store';
33
import thunk from 'redux-thunk';
44
import { TransactionStatus } from '@metamask/transaction-controller';
5+
import { act, waitFor } from '@testing-library/react';
56
import { GAS_LIMITS } from '../../../../shared/constants/gas';
67
import { renderWithProvider } from '../../../../test/lib/render-helpers';
78
import mockState from '../../../../test/data/mock-state.json';
@@ -44,7 +45,7 @@ const transactionGroup = {
4445
hasCancelled: false,
4546
};
4647

47-
const render = (overrideProps) => {
48+
const render = async (overrideProps) => {
4849
const rpcPrefs = {
4950
blockExplorerUrl: 'https://customblockexplorer.com/',
5051
};
@@ -61,54 +62,56 @@ const render = (overrideProps) => {
6162
senderAddress: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc',
6263
tryReverseResolveAddress: jest.fn(),
6364
transactionGroup,
64-
transactionStatus: () => <div />,
65+
transactionStatus: () => <div></div>,
6566
blockExplorerLinkText,
6667
rpcPrefs,
6768
...overrideProps,
6869
};
6970

7071
const mockStore = configureMockStore([thunk])(mockState);
7172

72-
const result = renderWithProvider(
73-
<TransactionListItemDetails {...props} />,
74-
mockStore,
73+
let result;
74+
75+
await act(
76+
async () =>
77+
(result = renderWithProvider(
78+
<TransactionListItemDetails {...props} />,
79+
mockStore,
80+
)),
7581
);
7682

7783
return result;
7884
};
7985

8086
describe('TransactionListItemDetails Component', () => {
81-
describe('matches snapshot', () => {
82-
it('for non-error details', async () => {
83-
const { queryByText, queryByTestId } = render();
84-
expect(queryByText('Test Transaction Details')).toBeInTheDocument();
85-
expect(
86-
queryByTestId('transaction-list-item-details-banner-error-message'),
87-
).not.toBeInTheDocument();
88-
});
87+
it('should render title with title prop', async () => {
88+
const { queryByText } = await render();
8989

90-
it('for error details', async () => {
91-
const { queryByText, queryByTestId } = render({ showErrorBanner: true });
90+
await waitFor(() => {
9291
expect(queryByText('Test Transaction Details')).toBeInTheDocument();
93-
expect(
94-
queryByTestId('transaction-list-item-details-banner-error-message'),
95-
).toBeInTheDocument();
9692
});
9793
});
9894

99-
describe('Action buttons', () => {
100-
it('renders retry button with showRetry prop', async () => {
101-
const { queryByTestId } = render({ showRetry: true });
95+
describe('Retry button', () => {
96+
it('should render retry button with showRetry prop', async () => {
97+
const { queryByTestId } = await render({ showRetry: true });
98+
10299
expect(queryByTestId('rety-button')).toBeInTheDocument();
103100
});
101+
});
102+
103+
describe('Cancel button', () => {
104+
it('should render cancel button with showCancel prop', async () => {
105+
const { queryByTestId } = await render({ showCancel: true });
104106

105-
it('renders cancel button with showCancel prop', async () => {
106-
const { queryByTestId } = render({ showCancel: true });
107107
expect(queryByTestId('cancel-button')).toBeInTheDocument();
108108
});
109+
});
110+
111+
describe('Speedup button', () => {
112+
it('should render speedup button with showSpeedUp prop', async () => {
113+
const { queryByTestId } = await render({ showSpeedUp: true });
109114

110-
it('renders speedup button with showSpeedUp prop', async () => {
111-
const { queryByTestId } = render({ showSpeedUp: true });
112115
expect(queryByTestId('speedup-button')).toBeInTheDocument();
113116
});
114117
});

ui/components/app/transaction-list-item/smart-transaction-list-item.component.js

-1
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,6 @@ export default function SmartTransactionListItem({
126126
date={date}
127127
status={displayedStatusKey}
128128
statusOnly
129-
shouldShowTooltip={false}
130129
/>
131130
)}
132131
chainId={chainId}

ui/components/app/transaction-list-item/transaction-list-item.component.js

-3
Original file line numberDiff line numberDiff line change
@@ -372,8 +372,6 @@ function TransactionListItemInner({
372372
showSpeedUp={shouldShowSpeedUp}
373373
isEarliestNonce={isEarliestNonce}
374374
onCancel={cancelTransaction}
375-
showCancel={isPending && !hasCancelled && !isBridgeTx}
376-
showErrorBanner={Boolean(error)}
377375
transactionStatus={() => (
378376
<TransactionStatusLabel
379377
isPending={isPending}
@@ -382,7 +380,6 @@ function TransactionListItemInner({
382380
date={date}
383381
status={displayedStatusKey}
384382
statusOnly
385-
shouldShowTooltip={false}
386383
/>
387384
)}
388385
chainId={chainId}

0 commit comments

Comments
 (0)