Skip to content

Commit b7c3f83

Browse files
pedronfigueiredopedronfigueiredo
authored
fix: Remove multiple overlapping spinners (#28301)
<!-- 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** We were previously using a UI blocking, whole page spinner while loading the redesigned confirmations. This overlapped with the individual component spinners we use on some components that decode transaction data. That second pattern is preferable because the spinner is contained and doesn't block the user from taking action. The global spinner comes from `routes.component`. The first part of this fix is to bypass it in that file for redesigned confirmations. The second aspect of this fix is to not condense two component spinners into one. Since the transaction flow section always loads before the send heading component, we can omit the spinner for that first component. Finally, this PR fixes the loading behaviour on the send heading component so that if the decimals amount coming from `useAssetDetails` hasn't been received yet, the heading is not shown, preventing undesireable flickering in the hero value. <!-- 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/28301?quickstart=1) ## **Related issues** Fixes: #27972 ## **Manual testing steps** 1. Go to test dapp 2. Click on malicious erc20 transfer to see the 2 spinners on top and the different one in the middle 3. Click on malicious erc20 approve to see the different spinner in the middle ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> <img width="362" alt="Screenshot 2024-10-18 at 18 11 08" src="https://github.com/user-attachments/assets/9fbc5c1e-229c-4dfa-8231-4957a1ecf713"> ### **After** <!-- [screenshots/recordings] --> https://github.com/user-attachments/assets/2f5dd8c2-3b9b-405d-b9b5-0eb8bf793fcb ## **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/develop/.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/develop/.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 c5c59df commit b7c3f83

File tree

14 files changed

+128
-229
lines changed

14 files changed

+128
-229
lines changed

‎ui/pages/confirmations/components/confirm/info/approve/hooks/use-approve-token-simulation.ts

+11-11
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
11
import { TransactionMeta } from '@metamask/transaction-controller';
22
import { isHexString } from '@metamask/utils';
3-
import { BigNumber } from 'bignumber.js';
43
import { isBoolean } from 'lodash';
54
import { useMemo } from 'react';
65
import { useSelector } from 'react-redux';
6+
import { calcTokenAmount } from '../../../../../../../../shared/lib/transactions-controller-utils';
77
import { getIntlLocale } from '../../../../../../../ducks/locale/locale';
88
import { SPENDING_CAP_UNLIMITED_MSG } from '../../../../../constants';
9-
import { toNonScientificString } from '../../hooks/use-token-values';
109
import { useDecodedTransactionData } from '../../hooks/useDecodedTransactionData';
1110
import { useIsNFT } from './use-is-nft';
1211

@@ -27,7 +26,7 @@ export const useApproveTokenSimulation = (
2726

2827
const decodedSpendingCap = useMemo(() => {
2928
if (!value) {
30-
return 0;
29+
return '0';
3130
}
3231

3332
const paramIndex = value.data[0].params.findIndex(
@@ -38,23 +37,24 @@ export const useApproveTokenSimulation = (
3837
!isBoolean(param.value),
3938
);
4039
if (paramIndex === -1) {
41-
return 0;
40+
return '0';
4241
}
4342

44-
return new BigNumber(value.data[0].params[paramIndex].value.toString())
45-
.dividedBy(new BigNumber(10).pow(Number(decimals)))
46-
.toNumber();
43+
return calcTokenAmount(
44+
value.data[0].params[paramIndex].value,
45+
Number(decimals),
46+
).toFixed();
4747
}, [value, decimals]);
4848

4949
const formattedSpendingCap = useMemo(() => {
5050
// formatting coerces small numbers to 0
51-
return isNFT || decodedSpendingCap < 1
52-
? toNonScientificString(decodedSpendingCap)
53-
: new Intl.NumberFormat(locale).format(decodedSpendingCap);
51+
return isNFT || parseInt(decodedSpendingCap, 10) < 1
52+
? decodedSpendingCap
53+
: new Intl.NumberFormat(locale).format(parseInt(decodedSpendingCap, 10));
5454
}, [decodedSpendingCap, isNFT, locale]);
5555

5656
const spendingCap = useMemo(() => {
57-
if (!isNFT && isSpendingCapUnlimited(decodedSpendingCap)) {
57+
if (!isNFT && isSpendingCapUnlimited(parseInt(decodedSpendingCap, 10))) {
5858
return SPENDING_CAP_UNLIMITED_MSG;
5959
}
6060
const tokenPrefix = isNFT ? '#' : '';

‎ui/pages/confirmations/components/confirm/info/hooks/use-token-values.test.ts

+1-19
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import mockState from '../../../../../../../test/data/mock-state.json';
55
import { renderHookWithConfirmContextProvider } from '../../../../../../../test/lib/confirmations/render-helpers';
66
import useTokenExchangeRate from '../../../../../../components/app/currency-input/hooks/useTokenExchangeRate';
77
import { useAssetDetails } from '../../../../hooks/useAssetDetails';
8-
import { toNonScientificString, useTokenValues } from './use-token-values';
8+
import { useTokenValues } from './use-token-values';
99
import { useDecodedTransactionData } from './useDecodedTransactionData';
1010

1111
jest.mock('../../../../hooks/useAssetDetails', () => ({
@@ -126,21 +126,3 @@ describe('useTokenValues', () => {
126126
});
127127
});
128128
});
129-
130-
describe('toNonScientificString', () => {
131-
const TEST_CASES = [
132-
{ scientific: 1.23e-5, expanded: '0.0000123' },
133-
{ scientific: 1e-10, expanded: '0.0000000001' },
134-
{ scientific: 1.23e-21, expanded: '1.23e-21' },
135-
];
136-
137-
// @ts-expect-error This is missing from the Mocha type definitions
138-
it.each(TEST_CASES)(
139-
'Expand $scientific to "$expanded"',
140-
({ scientific, expanded }: { scientific: number; expanded: string }) => {
141-
const actual = toNonScientificString(scientific);
142-
143-
expect(actual).toEqual(expanded);
144-
},
145-
);
146-
});

‎ui/pages/confirmations/components/confirm/info/hooks/use-token-values.ts

+40-29
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { BigNumber } from 'bignumber.js';
44
import { isBoolean } from 'lodash';
55
import { useMemo, useState } from 'react';
66
import { useSelector } from 'react-redux';
7+
import { calcTokenAmount } from '../../../../../../../shared/lib/transactions-controller-utils';
78
import { Numeric } from '../../../../../../../shared/modules/Numeric';
89
import useTokenExchangeRate from '../../../../../../components/app/currency-input/hooks/useTokenExchangeRate';
910
import { getIntlLocale } from '../../../../../../ducks/locale/locale';
@@ -23,26 +24,45 @@ export const useTokenValues = (transactionMeta: TransactionMeta) => {
2324
const decodedResponse = useDecodedTransactionData();
2425
const { value, pending } = decodedResponse;
2526

26-
const decodedTransferValue = useMemo(() => {
27-
if (!value || !decimals) {
28-
return 0;
29-
}
27+
const { decodedTransferValue, isDecodedTransferValuePending } =
28+
useMemo(() => {
29+
if (!value) {
30+
return {
31+
decodedTransferValue: '0',
32+
isDecodedTransferValuePending: false,
33+
};
34+
}
3035

31-
const paramIndex = value.data[0].params.findIndex(
32-
(param) =>
33-
param.value !== undefined &&
34-
!isHexString(param.value) &&
35-
param.value.length === undefined &&
36-
!isBoolean(param.value),
37-
);
38-
if (paramIndex === -1) {
39-
return 0;
40-
}
36+
if (!decimals) {
37+
return {
38+
decodedTransferValue: '0',
39+
isDecodedTransferValuePending: true,
40+
};
41+
}
4142

42-
return new BigNumber(value.data[0].params[paramIndex].value.toString())
43-
.dividedBy(new BigNumber(10).pow(Number(decimals)))
44-
.toNumber();
45-
}, [value, decimals]);
43+
const paramIndex = value.data[0].params.findIndex(
44+
(param) =>
45+
param.value !== undefined &&
46+
!isHexString(param.value) &&
47+
param.value.length === undefined &&
48+
!isBoolean(param.value),
49+
);
50+
if (paramIndex === -1) {
51+
return {
52+
decodedTransferValue: '0',
53+
isDecodedTransferValuePending: false,
54+
};
55+
}
56+
57+
return {
58+
decodedTransferValue: calcTokenAmount(
59+
value.data[0].params[paramIndex].value,
60+
decimals,
61+
).toFixed(),
62+
isDecodedTransferValuePending: false,
63+
};
64+
// };
65+
}, [value, decimals]);
4666

4767
const [exchangeRate, setExchangeRate] = useState<Numeric | undefined>();
4868
const fetchExchangeRate = async () => {
@@ -67,18 +87,9 @@ export const useTokenValues = (transactionMeta: TransactionMeta) => {
6787
);
6888

6989
return {
70-
decodedTransferValue: toNonScientificString(decodedTransferValue),
90+
decodedTransferValue,
7191
displayTransferValue,
7292
fiatDisplayValue,
73-
pending,
93+
pending: pending || isDecodedTransferValuePending,
7494
};
7595
};
76-
77-
export function toNonScientificString(num: number): string {
78-
if (num >= 10e-18) {
79-
return num.toFixed(18).replace(/\.?0+$/u, '');
80-
}
81-
82-
// keep in scientific notation
83-
return num.toString();
84-
}

‎ui/pages/confirmations/components/confirm/info/native-transfer/__snapshots__/native-transfer.test.tsx.snap

-43
Original file line numberDiff line numberDiff line change
@@ -16,49 +16,6 @@ exports[`NativeTransferInfo renders correctly 1`] = `
1616
0 ETH
1717
</h2>
1818
</div>
19-
<div
20-
class="mm-box mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
21-
>
22-
<svg
23-
class="preloader__icon"
24-
fill="none"
25-
height="20"
26-
viewBox="0 0 16 16"
27-
width="20"
28-
xmlns="http://www.w3.org/2000/svg"
29-
>
30-
<path
31-
clip-rule="evenodd"
32-
d="M8 13.7143C4.84409 13.7143 2.28571 11.1559 2.28571 8C2.28571 4.84409 4.84409 2.28571 8 2.28571C11.1559 2.28571 13.7143 4.84409 13.7143 8C13.7143 11.1559 11.1559 13.7143 8 13.7143ZM8 16C3.58172 16 0 12.4183 0 8C0 3.58172 3.58172 0 8 0C12.4183 0 16 3.58172 16 8C16 12.4183 12.4183 16 8 16Z"
33-
fill="var(--color-primary-muted)"
34-
fill-rule="evenodd"
35-
/>
36-
<mask
37-
height="16"
38-
id="mask0"
39-
mask-type="alpha"
40-
maskUnits="userSpaceOnUse"
41-
width="16"
42-
x="0"
43-
y="0"
44-
>
45-
<path
46-
clip-rule="evenodd"
47-
d="M8 13.7143C4.84409 13.7143 2.28571 11.1559 2.28571 8C2.28571 4.84409 4.84409 2.28571 8 2.28571C11.1559 2.28571 13.7143 4.84409 13.7143 8C13.7143 11.1559 11.1559 13.7143 8 13.7143ZM8 16C3.58172 16 0 12.4183 0 8C0 3.58172 3.58172 0 8 0C12.4183 0 16 3.58172 16 8C16 12.4183 12.4183 16 8 16Z"
48-
fill="var(--color-primary-default)"
49-
fill-rule="evenodd"
50-
/>
51-
</mask>
52-
<g
53-
mask="url(#mask0)"
54-
>
55-
<path
56-
d="M6.85718 17.9999V11.4285V8.28564H-4.85711V17.9999H6.85718Z"
57-
fill="var(--color-primary-default)"
58-
/>
59-
</g>
60-
</svg>
61-
</div>
6219
<div
6320
class="mm-box mm-box--margin-bottom-4 mm-box--padding-0 mm-box--background-color-background-default mm-box--rounded-md"
6421
>

‎ui/pages/confirmations/components/confirm/info/nft-token-transfer/__snapshots__/nft-token-transfer.test.tsx.snap

-43
Original file line numberDiff line numberDiff line change
@@ -17,49 +17,6 @@ exports[`NFTTokenTransferInfo renders correctly 1`] = `
1717
class="mm-box mm-text mm-text--body-md mm-box--color-text-alternative"
1818
/>
1919
</div>
20-
<div
21-
class="mm-box mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
22-
>
23-
<svg
24-
class="preloader__icon"
25-
fill="none"
26-
height="20"
27-
viewBox="0 0 16 16"
28-
width="20"
29-
xmlns="http://www.w3.org/2000/svg"
30-
>
31-
<path
32-
clip-rule="evenodd"
33-
d="M8 13.7143C4.84409 13.7143 2.28571 11.1559 2.28571 8C2.28571 4.84409 4.84409 2.28571 8 2.28571C11.1559 2.28571 13.7143 4.84409 13.7143 8C13.7143 11.1559 11.1559 13.7143 8 13.7143ZM8 16C3.58172 16 0 12.4183 0 8C0 3.58172 3.58172 0 8 0C12.4183 0 16 3.58172 16 8C16 12.4183 12.4183 16 8 16Z"
34-
fill="var(--color-primary-muted)"
35-
fill-rule="evenodd"
36-
/>
37-
<mask
38-
height="16"
39-
id="mask0"
40-
mask-type="alpha"
41-
maskUnits="userSpaceOnUse"
42-
width="16"
43-
x="0"
44-
y="0"
45-
>
46-
<path
47-
clip-rule="evenodd"
48-
d="M8 13.7143C4.84409 13.7143 2.28571 11.1559 2.28571 8C2.28571 4.84409 4.84409 2.28571 8 2.28571C11.1559 2.28571 13.7143 4.84409 13.7143 8C13.7143 11.1559 11.1559 13.7143 8 13.7143ZM8 16C3.58172 16 0 12.4183 0 8C0 3.58172 3.58172 0 8 0C12.4183 0 16 3.58172 16 8C16 12.4183 12.4183 16 8 16Z"
49-
fill="var(--color-primary-default)"
50-
fill-rule="evenodd"
51-
/>
52-
</mask>
53-
<g
54-
mask="url(#mask0)"
55-
>
56-
<path
57-
d="M6.85718 17.9999V11.4285V8.28564H-4.85711V17.9999H6.85718Z"
58-
fill="var(--color-primary-default)"
59-
/>
60-
</g>
61-
</svg>
62-
</div>
6320
<div
6421
class="mm-box mm-box--margin-bottom-4 mm-box--padding-0 mm-box--background-color-background-default mm-box--rounded-md"
6522
>

‎ui/pages/confirmations/components/confirm/info/shared/confirm-loader/__snapshots__/confirm-loader.test.tsx.snap

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
exports[`<ConfirmLoader /> renders component 1`] = `
44
<div>
55
<div
6-
class="mm-box mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
6+
class="mm-box mm-box--padding-top-4 mm-box--padding-bottom-4 mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
77
>
88
<svg
99
class="preloader__icon"

‎ui/pages/confirmations/components/confirm/info/shared/confirm-loader/confirm-loader.tsx

+2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ export const ConfirmLoader = () => {
1313
display={Display.Flex}
1414
justifyContent={JustifyContent.center}
1515
alignItems={AlignItems.center}
16+
paddingTop={4}
17+
paddingBottom={4}
1618
>
1719
<Preloader size={20} />
1820
</Box>

‎ui/pages/confirmations/components/confirm/info/shared/native-send-heading/native-send-heading.tsx

+15-21
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import {
66
CHAIN_ID_TO_NETWORK_IMAGE_URL_MAP,
77
TEST_CHAINS,
88
} from '../../../../../../../../shared/constants/network';
9+
import { calcTokenAmount } from '../../../../../../../../shared/lib/transactions-controller-utils';
910
import {
1011
AvatarToken,
1112
AvatarTokenSize,
@@ -22,29 +23,25 @@ import {
2223
TextColor,
2324
TextVariant,
2425
} from '../../../../../../../helpers/constants/design-system';
25-
import { MIN_AMOUNT } from '../../../../../../../hooks/useCurrencyDisplay';
2626
import { useFiatFormatter } from '../../../../../../../hooks/useFiatFormatter';
2727
import {
2828
getPreferences,
2929
selectConversionRateByChainId,
3030
} from '../../../../../../../selectors';
3131
import { getMultichainNetwork } from '../../../../../../../selectors/multichain';
3232
import { useConfirmContext } from '../../../../../context/confirm';
33-
import {
34-
formatAmount,
35-
formatAmountMaxPrecision,
36-
} from '../../../../simulation-details/formatAmount';
37-
import { toNonScientificString } from '../../hooks/use-token-values';
33+
import { formatAmount } from '../../../../simulation-details/formatAmount';
3834

3935
const NativeSendHeading = () => {
4036
const { currentConfirmation: transactionMeta } =
4137
useConfirmContext<TransactionMeta>();
4238

4339
const { chainId } = transactionMeta;
4440

45-
const nativeAssetTransferValue = new BigNumber(
41+
const nativeAssetTransferValue = calcTokenAmount(
4642
transactionMeta.txParams.value as string,
47-
).dividedBy(new BigNumber(10).pow(18));
43+
18,
44+
);
4845

4946
const conversionRate = useSelector((state) =>
5047
selectConversionRateByChainId(state, chainId),
@@ -66,9 +63,7 @@ const NativeSendHeading = () => {
6663
const locale = useSelector(getIntlLocale);
6764
const roundedTransferValue = formatAmount(locale, nativeAssetTransferValue);
6865

69-
const transferValue = toNonScientificString(
70-
nativeAssetTransferValue.toNumber(),
71-
);
66+
const transferValue = nativeAssetTransferValue.toFixed();
7267

7368
type TestNetChainId = (typeof TEST_CHAINS)[number];
7469
const isTestnet = TEST_CHAINS.includes(
@@ -90,8 +85,15 @@ const NativeSendHeading = () => {
9085
);
9186

9287
const NativeAssetAmount =
93-
roundedTransferValue ===
94-
`<${formatAmountMaxPrecision(locale, MIN_AMOUNT)}` ? (
88+
roundedTransferValue === transferValue ? (
89+
<Text
90+
variant={TextVariant.headingLg}
91+
color={TextColor.inherit}
92+
marginTop={3}
93+
>
94+
{`${roundedTransferValue} ${ticker}`}
95+
</Text>
96+
) : (
9597
<Tooltip title={transferValue} position="right">
9698
<Text
9799
variant={TextVariant.headingLg}
@@ -101,14 +103,6 @@ const NativeSendHeading = () => {
101103
{`${roundedTransferValue} ${ticker}`}
102104
</Text>
103105
</Tooltip>
104-
) : (
105-
<Text
106-
variant={TextVariant.headingLg}
107-
color={TextColor.inherit}
108-
marginTop={3}
109-
>
110-
{`${roundedTransferValue} ${ticker}`}
111-
</Text>
112106
);
113107

114108
const NativeAssetFiatConversion = Boolean(fiatDisplayValue) &&

‎ui/pages/confirmations/components/confirm/info/shared/send-heading/__snapshots__/send-heading.test.tsx.snap

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
exports[`<SendHeading /> renders component 1`] = `
44
<div>
55
<div
6-
class="mm-box mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
6+
class="mm-box mm-box--padding-top-4 mm-box--padding-bottom-4 mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center"
77
>
88
<svg
99
class="preloader__icon"

0 commit comments

Comments
 (0)