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: Update Redesign Signature Permit to show ellipsis at max 15 digits #26227

Merged
merged 5 commits into from
Aug 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions ui/components/app/confirm/info/row/text-token-units.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,46 @@ describe('ConfirmInfoRowTextTokenUnits', () => {
<ConfirmInfoRowTextTokenUnits value={value} decimals={decimals} />,
);

// Note: using formatAmount loses precision
expect(getByText('0.0123')).toBeInTheDocument();
});

it('renders the value with the correct formatted number for lengthy decimals', () => {
const value = '300012312312123121';
const decimals = 18;
const { getByText } = render(
<ConfirmInfoRowTextTokenUnits value={value} decimals={decimals} />,
);

expect(getByText('0.3')).toBeInTheDocument();
});

it('renders the value with the correct formatted non-fractional number', () => {
const value = 123456789;
const decimals = 4;
const { getByText } = render(
<ConfirmInfoRowTextTokenUnits value={value} decimals={decimals} />,
);

// Note: using formatAmount loses precision
expect(getByText('12,346')).toBeInTheDocument();
});

it('renders the value with the correct formatted number', () => {
const value = '30001231231212312138768';
const decimals = 9;
const { getByText } = render(
<ConfirmInfoRowTextTokenUnits value={value} decimals={decimals} />,
);

expect(getByText('30,001,231,231,212')).toBeInTheDocument();
});

it('renders the value with the correct formatted number and ellipsis', () => {
const value = '30001231231212312138768';
const decimals = 7;
const { getByText } = render(
<ConfirmInfoRowTextTokenUnits value={value} decimals={decimals} />,
);

expect(getByText('3,000,123,123,121,23...')).toBeInTheDocument();
});
});
6 changes: 2 additions & 4 deletions ui/components/app/confirm/info/row/text-token-units.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { BigNumber } from 'bignumber.js';

import { calcTokenAmount } from '../../../../../../shared/lib/transactions-controller-utils';
import {
ellipsisAmountText,
formatAmount,
formatAmountMaxPrecision,
} from '../../../../../pages/confirmations/components/simulation-details/formatAmount';
Expand All @@ -18,15 +19,12 @@ export const ConfirmInfoRowTextTokenUnits: React.FC<
> = ({ value, decimals }) => {
const tokenValue = calcTokenAmount(value, decimals);

// FIXME - Precision may be lost for large values when using formatAmount
/** @see {@link https://github.com/MetaMask/metamask-extension/issues/25755} */
const tokenText = formatAmount('en-US', tokenValue);
const tokenTextMaxPrecision = formatAmountMaxPrecision('en-US', tokenValue);

return (
<ConfirmInfoRowText
isEllipsis={true}
text={tokenText}
text={ellipsisAmountText(tokenText)}
tooltip={tokenTextMaxPrecision}
/>
);
Expand Down
20 changes: 4 additions & 16 deletions ui/components/app/confirm/info/row/text.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,8 @@ import {
} from '../../../../component-library';
import Tooltip from '../../../../ui/tooltip';

const InfoText = ({
isEllipsis,
text,
}: {
isEllipsis: boolean;
text: string;
}) => (
<Text
color={TextColor.inherit}
style={isEllipsis ? {} : { whiteSpace: 'pre-wrap' }}
ellipsis={isEllipsis}
>
const InfoText = ({ text }: { text: string }) => (
<Text color={TextColor.inherit} style={{ whiteSpace: 'pre-wrap' }}>
{text}
</Text>
);
Expand All @@ -37,14 +27,12 @@ export type ConfirmInfoRowTextProps = {
text: string;
onEditClick?: () => void;
editIconClassName?: string;
isEllipsis?: boolean;
tooltip?: string;
};

export const ConfirmInfoRowText: React.FC<ConfirmInfoRowTextProps> = ({
text,
onEditClick,
isEllipsis = false,
editIconClassName,
tooltip,
}) => {
Expand All @@ -67,10 +55,10 @@ export const ConfirmInfoRowText: React.FC<ConfirmInfoRowTextProps> = ({
wrapperStyle={{ minWidth: 0 }}
interactive
>
<InfoText isEllipsis={isEllipsis} text={text} />
<InfoText text={text} />
</Tooltip>
) : (
<InfoText isEllipsis={isEllipsis} text={text} />
<InfoText text={text} />
)}
{isEditable ? (
<ButtonIcon
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,8 @@ exports[`TypedSignInfo correctly renders permit sign type 1`] = `
tabindex="0"
>
<p
class="mm-box mm-text mm-text--body-md mm-text--ellipsis mm-box--color-inherit"
class="mm-box mm-text mm-text--body-md mm-box--color-inherit"
style="white-space: pre-wrap;"
>
3,000
</p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ exports[`PermitSimulation renders component correctly 1`] = `
tabindex="0"
>
<p
class="mm-box mm-text mm-text--body-md mm-text--ellipsis mm-text--text-align-center mm-box--padding-inline-2 mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-xl"
class="mm-box mm-text mm-text--body-md mm-text--text-align-center mm-box--padding-inline-2 mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-xl"
data-testid="simulation-token-value"
>
30
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { SignatureRequestType } from '../../../../../types/confirm';
import useTokenExchangeRate from '../../../../../../../components/app/currency-input/hooks/useTokenExchangeRate';
import { IndividualFiatDisplay } from '../../../../simulation-details/fiat-display';
import {
ellipsisAmountText,
formatAmount,
formatAmountMaxPrecision,
} from '../../../../simulation-details/formatAmount';
Expand Down Expand Up @@ -93,9 +94,8 @@ const PermitSimulation: React.FC<{
borderRadius={BorderRadius.XL}
paddingInline={2}
textAlign={TextAlign.Center}
ellipsis
>
{tokenValue}
{ellipsisAmountText(tokenValue || '')}
</Text>
</Tooltip>
</Box>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,51 +1,70 @@
import { BigNumber } from 'bignumber.js';
import { formatAmount } from './formatAmount';
import { ellipsisAmountText, formatAmount } from './formatAmount';

describe('formatAmount', () => {
const locale = 'en-US';
describe('#ellipsisAmountText', () => {
const MOCK_MAX_LEFT_DIGITS = 15;

it('returns "0" for zero amount', () => {
expect(formatAmount(locale, new BigNumber(0))).toBe('0');
// @ts-expect-error This is missing from the Mocha type definitions
it.each([
['1.003', '1.003'],
['1,034', '1,034'],
['1,213,098,292,340,945', '1,213,098,292,340,94...'],
['30,001,231,231,212,312,138,768', '30,001,231,231,212,3...'],
])(
'formats amount greater than or equal to 1 with appropriate decimal precision (%s => %s)',
(amount: string, expected: string) => {
expect(ellipsisAmountText(amount, MOCK_MAX_LEFT_DIGITS)).toBe(expected);
},
);
});

it('returns "<0.000001" for 0 < amount < MIN_AMOUNT', () => {
expect(formatAmount(locale, new BigNumber(0.0000009))).toBe('<0.000001');
});
describe('#formatAmount', () => {
const locale = 'en-US';

it('returns "0" for zero amount', () => {
expect(formatAmount(locale, new BigNumber(0))).toBe('0');
});

// @ts-expect-error This is missing from the Mocha type definitions
it.each([
[0.0000456, '0.000046'],
[0.0004567, '0.000457'],
[0.003456, '0.00346'],
[0.023456, '0.0235'],
[0.125456, '0.125'],
])(
'formats amount less than 1 with maximum significant digits (%s => %s)',
(amount: number, expected: string) => {
expect(formatAmount(locale, new BigNumber(amount))).toBe(expected);
},
);
it('returns "<0.000001" for 0 < amount < MIN_AMOUNT', () => {
expect(formatAmount(locale, new BigNumber(0.0000009))).toBe('<0.000001');
});

// @ts-expect-error This is missing from the Mocha type definitions
it.each([
[1.0034, '1.003'],
[1.034, '1.034'],
[1.3034, '1.303'],
[12.0345, '12.03'],
[121.456, '121.5'],
[1034.123, '1,034'],
[47361034.006, '47,361,034'],
['12130982923409.5', '12,130,982,923,410'],
['1213098292340944.5', '1,213,098,292,340,945'],
['30001231231212312138768', '30,001,231,231,212,312,138,768'],
[
'1157920892373161954235709850086879078532699846656405640394575.84007913129639935',
'1,157,920,892,373,161,954,235,709,850,086,879,078,532,699,846,656,405,640,394,576',
],
])(
'formats amount greater than or equal to 1 with appropriate decimal precision (%s => %s)',
(amount: number, expected: string) => {
expect(formatAmount(locale, new BigNumber(amount))).toBe(expected);
},
);
// @ts-expect-error This is missing from the Mocha type definitions
it.each([
[0.0000456, '0.000046'],
[0.0004567, '0.000457'],
[0.003456, '0.00346'],
[0.023456, '0.0235'],
[0.125456, '0.125'],
])(
'formats amount less than 1 with maximum significant digits (%s => %s)',
(amount: number, expected: string) => {
expect(formatAmount(locale, new BigNumber(amount))).toBe(expected);
},
);

// @ts-expect-error This is missing from the Mocha type definitions
it.each([
[1.0034, '1.003'],
[1.034, '1.034'],
[1.3034, '1.303'],
[12.0345, '12.03'],
[121.456, '121.5'],
[1034.123, '1,034'],
[47361034.006, '47,361,034'],
['12130982923409.5', '12,130,982,923,410'],
['1213098292340944.5', '1,213,098,292,340,945'],
['30001231231212312138768', '30,001,231,231,212,312,138,768'],
[
'1157920892373161954235709850086879078532699846656405640394575.84007913129639935',
'1,157,920,892,373,161,954,235,709,850,086,879,078,532,699,846,656,405,640,394,576',
],
])(
'formats amount greater than or equal to 1 with appropriate decimal precision (%s => %s)',
(amount: number, expected: string) => {
expect(formatAmount(locale, new BigNumber(amount))).toBe(expected);
},
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,51 @@ import {
DEFAULT_PRECISION,
} from '../../../../hooks/useCurrencyDisplay';

const MAX_ELLIPSIS_LEFT_DIGITS = 15;

// The number of significant decimals places to show for amounts less than 1.
const MAX_SIGNIFICANT_DECIMAL_PLACES = 3;

const ZERO_DISPLAY = '0';

/**
* This function receives an formatted number and will append an ellipsis if the number of digits
* is greater than MAX_LEFT_DIGITS. Currently, we're only supporting en-US format. When we support
* i18n numbers, we'll need to update this method to support i18n.
*
* This method has been designed to receive results of formatAmount.
*
* There is no need to format the decimal portion because formatAmount shaves the portions off
* accordingly.
*
* @param amountText
* @param maxLeftDigits
*/
export function ellipsisAmountText(
amountText: string,
maxLeftDigits: number = MAX_ELLIPSIS_LEFT_DIGITS,
): string {
const [integerPart] = amountText.split('.');
const cleanIntegerPart = integerPart.replace(/,/gu, '');

if (cleanIntegerPart.length > maxLeftDigits) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we re-use Goktug's shortenString function here?

From ui/helpers/utils/util.js.

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 think it’s better to keep it as is since this requires more logic to count the digits and preserve the “,”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I lost the design and issue ticket for this and ended up created a new ticket. Now that I've seen the relevant issue, we needed to do max 15 characters and not digits
https://github.com/MetaMask/MetaMask-planning/issues/2845

Updated to use shortenString here: #26458

let result = '';
let digitCount = 0;

for (let i = 0; digitCount < maxLeftDigits; i++) {
const integerChar = integerPart[i];
result += integerChar;
if (integerChar !== ',') {
digitCount += 1;
}
}

return `${result}...`;
}

return amountText;
}

export function formatAmountMaxPrecision(
locale: string,
num: number | BigNumber,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1675,7 +1675,8 @@ exports[`Confirm should match snapshot for signature - typed sign - permit 1`] =
tabindex="0"
>
<p
class="mm-box mm-text mm-text--body-md mm-text--ellipsis mm-box--color-inherit"
class="mm-box mm-text mm-text--body-md mm-box--color-inherit"
style="white-space: pre-wrap;"
>
3,000
</p>
Expand Down