Skip to content

Commit

Permalink
fix: asset picker bug (#25601)
Browse files Browse the repository at this point in the history
<!--
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**
the goal of this PR is to fix some v12.1 bugs related to balance display
and asset picker modal:
- The symbols no longer show for each list item
- Blocked token symbols (e.g., BNB on Ethereum Mainnet) are no longer
greyed out in the UI despite still being inert
- Token names are not truncated
- Token symbol is USD instead of ETH on the asset picker modal
<!--
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/25601?quickstart=1)

## **Related issues**

Fixes: #25575 

## **Manual testing steps**

1. Go to the wallet view and check crypto and fiat value for native
token
2. Select Ethereum Mainnet
3. Go to Send page
4. Click on destination asset picker
5. Observe that symbols are not present and names are not truncated
6. Search for "CHZ"
7. Observe that it is not styled as a disabled button

## **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**

- [x] 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).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] 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.
  • Loading branch information
salimtb authored Jul 3, 2024
1 parent b372e83 commit 92ea1c9
Show file tree
Hide file tree
Showing 10 changed files with 161 additions and 52 deletions.
92 changes: 92 additions & 0 deletions test/e2e/tests/multichain/asset-picker-send.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { strict as assert } from 'assert';
import { Context } from 'mocha';
import { CHAIN_IDS } from '../../../../shared/constants/network';
import FixtureBuilder from '../../fixture-builder';
import {
defaultGanacheOptions,
openActionMenuAndStartSendFlow,
unlockWallet,
withFixtures,
} from '../../helpers';
import { Driver } from '../../webdriver/driver';
import { RECIPIENT_ADDRESS_MOCK } from '../simulation-details/types';

describe('AssetPickerSendFlow', function () {
const chainId = CHAIN_IDS.MAINNET;

const fixtures = {
fixtures: new FixtureBuilder({ inputChainId: chainId }).build(),
ganacheOptions: {
...defaultGanacheOptions,
chainId: parseInt(chainId, 16),
},
};

it('should send token using asset picker modal', async function () {
const ethConversionInUsd = 10000;

await withFixtures(
{
...fixtures,
title: (this as Context).test?.fullTitle(),
ethConversionInUsd,
},
async ({ driver }: { driver: Driver }) => {
await unlockWallet(driver);

// Open the send flow
openActionMenuAndStartSendFlow(driver);

await driver.fill('[data-testid="ens-input"]', RECIPIENT_ADDRESS_MOCK);
await driver.fill('.unit-input__input', '2');

const isDest = 'dest';
const buttons = await driver.findElements(
'[data-testid="asset-picker-button"]',
);
const indexOfButtonToClick = isDest ? 1 : 0;
await buttons[indexOfButtonToClick].click();

// check that the name , crypto amount and fiat amount are correctly displayed
const tokenListName = await (
await driver.findElement(
'[data-testid="multichain-token-list-item-token-name"]',
)
).getText();

assert.equal(tokenListName, 'Ethereum');

const tokenListValue = await (
await driver.findElement(
'[data-testid="multichain-token-list-item-value"]',
)
).getText();

assert.equal(tokenListValue, '25 ETH');

const tokenListSecondaryValue = await (
await driver.findElement(
'[data-testid="multichain-token-list-item-secondary-value"]',
)
).getText();

assert.equal(tokenListSecondaryValue, '$250,000.00');

// Search for BNB
const searchInputField = await driver.waitForSelector(
'[data-testid="asset-picker-modal-search-input"]',
);
await searchInputField.sendKeys('CHZ');

// check that BNB is disabled
const [, tkn] = await driver.findElements(
'[data-testid="multichain-token-list-button"]',
);

await tkn.click();
const isSelected = await tkn.isSelected();
assert.equal(isSelected, false);
},
);
});
});
1 change: 1 addition & 0 deletions ui/components/app/asset-list/asset-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ const AssetList = ({ onClickAsset, showTokensLinks }) => {
isOriginalTokenSymbol={isOriginalNativeSymbol}
isNativeCurrency
isStakeable={isStakeable}
showPercentage
/>
<TokenList
tokens={tokensWithBalances}
Expand Down
1 change: 1 addition & 0 deletions ui/components/app/token-cell/token-cell.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export default function TokenCell({ address, image, symbol, string, onClick }) {
title={title}
isOriginalTokenSymbol={isOriginalTokenSymbol}
address={address}
showPercentage
/>
);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';
import { useSelector } from 'react-redux';
import { getSelectedAccountCachedBalance } from '../../../../selectors';
import {
getPreferences,
getSelectedAccountCachedBalance,
} from '../../../../selectors';
import { getNativeCurrency } from '../../../../ducks/metamask/metamask';
import { useUserPreferencedCurrency } from '../../../../hooks/useUserPreferencedCurrency';
import { useCurrencyDisplay } from '../../../../hooks/useCurrencyDisplay';
Expand Down Expand Up @@ -78,6 +81,9 @@ describe('AssetList', () => {
if (selector === getSelectedAccountCachedBalance) {
return balanceValue;
}
if (selector === getPreferences) {
return true;
}
return undefined;
});

Expand Down Expand Up @@ -136,6 +142,9 @@ describe('AssetList', () => {
if (selector === getSelectedAccountCachedBalance) {
return balanceValue;
}
if (selector === getPreferences) {
return true;
}
return undefined;
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import React from 'react';
import { useSelector } from 'react-redux';
import classnames from 'classnames';
import { getSelectedAccountCachedBalance } from '../../../../selectors';
import {
getPreferences,
getSelectedAccountCachedBalance,
} from '../../../../selectors';
import { getNativeCurrency } from '../../../../ducks/metamask/metamask';
import { useUserPreferencedCurrency } from '../../../../hooks/useUserPreferencedCurrency';
import { PRIMARY, SECONDARY } from '../../../../helpers/constants/common';
Expand Down Expand Up @@ -39,6 +42,7 @@ export default function AssetList({

const nativeCurrency = useSelector(getNativeCurrency);
const balanceValue = useSelector(getSelectedAccountCachedBalance);
const { useNativeCurrencyAsPrimaryCurrency } = useSelector(getPreferences);

const {
currency: primaryCurrency,
Expand Down Expand Up @@ -107,7 +111,6 @@ export default function AssetList({
display={Display.Block}
flexWrap={FlexWrap.NoWrap}
alignItems={AlignItems.center}
style={{ cursor: 'pointer' }}
>
<Box marginInlineStart={2}>
{token.type === AssetType.native ? (
Expand All @@ -117,9 +120,14 @@ export default function AssetList({
primaryCurrencyProperties.value ??
secondaryCurrencyProperties.value
}
tokenSymbol={primaryCurrencyProperties.suffix}
tokenSymbol={
useNativeCurrencyAsPrimaryCurrency
? primaryCurrency
: secondaryCurrency
}
secondary={secondaryCurrencyDisplay}
tokenImage={token.image}
isOriginalTokenSymbol
/>
) : (
<AssetComponent
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@
}
}

&--disabled {
@include disabled-token {
opacity: 0.4;
}
&--disabled,
&:disabled {
opacity: var(--opacity-disabled);
cursor: not-allowed;
}

&__selected-indicator {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,10 @@ exports[`TokenListItem should render correctly 1`] = `
<span
class="mm-box mm-text mm-text--body-md mm-text--font-weight-medium mm-text--ellipsis mm-box--color-text-default"
/>
<div
class="mm-box mm-box--display-flex"
>
<p
class="mm-box mm-text mm-text--body-md mm-text--font-weight-normal mm-text--ellipsis mm-box--color-text-default"
data-testid="token-increase-decrease-percentage-null"
/>
</div>
<p
class="mm-box mm-text mm-text--body-md mm-text--ellipsis mm-box--color-text-alternative"
data-testid="multichain-token-list-item-token-name"
/>
</div>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-column mm-box--align-items-flex-end mm-box--width-2/3"
Expand Down
61 changes: 32 additions & 29 deletions ui/components/multichain/token-list-item/token-list-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import { ModalContent } from '../../component-library/modal-content/deprecated';
import { ModalHeader } from '../../component-library/modal-header/deprecated';
import {
getMetaMetricsId,
getPreferences,
getTestNetworkBackgroundColor,
getTokensMarketData,
} from '../../../selectors';
Expand Down Expand Up @@ -77,6 +76,7 @@ export const TokenListItem = ({
isNativeCurrency = false,
isStakeable = false,
address = null,
showPercentage = false,
}) => {
const t = useI18nContext();
const isEvm = useSelector(getMultichainIsEvm);
Expand All @@ -86,13 +86,13 @@ export const TokenListItem = ({
const chainId = useSelector(getMultichainCurrentChainId);

// Scam warning
const showScamWarning = isNativeCurrency && !isOriginalTokenSymbol;
const showScamWarning =
isNativeCurrency && !isOriginalTokenSymbol && showPercentage;

const dispatch = useDispatch();
const [showScamWarningModal, setShowScamWarningModal] = useState(false);
const environmentType = getEnvironmentType();
const providerConfig = useSelector(getProviderConfig);
const { useNativeCurrencyAsPrimaryCurrency } = useSelector(getPreferences);
const isFullScreen = environmentType === ENVIRONMENT_TYPE_FULLSCREEN;
const history = useHistory();

Expand All @@ -117,6 +117,8 @@ export const TokenListItem = ({
tokensMarketData?.[address]?.pricePercentChange1d;

const tokenTitle = getTokenTitle();
const tokenMainTitleToDisplay = showPercentage ? tokenTitle : tokenSymbol;

const stakeableTitle = (
<Box
as="button"
Expand Down Expand Up @@ -257,10 +259,10 @@ export const TokenListItem = ({
>
{isStakeable ? (
<>
{tokenTitle} {stakeableTitle}
{tokenMainTitleToDisplay} {stakeableTitle}
</>
) : (
tokenTitle
tokenMainTitleToDisplay
)}
</Text>
</Tooltip>
Expand All @@ -273,15 +275,25 @@ export const TokenListItem = ({
>
{isStakeable ? (
<Box display={Display.InlineBlock}>
{tokenTitle} {stakeableTitle}
{tokenMainTitleToDisplay}
{stakeableTitle}
</Box>
) : (
tokenTitle
tokenMainTitleToDisplay
)}
</Text>
)}

{isEvm && (
{isEvm && !showPercentage ? (
<Text
variant={TextVariant.bodyMd}
color={TextColor.textAlternative}
data-testid="multichain-token-list-item-token-name"
ellipsis
>
{tokenTitle}
</Text>
) : (
<PercentageChange
value={
isNativeCurrency
Expand Down Expand Up @@ -313,27 +325,14 @@ export const TokenListItem = ({
data-testid="scam-warning"
/>

{useNativeCurrencyAsPrimaryCurrency ? (
<Text
fontWeight={FontWeight.Medium}
variant={TextVariant.bodyMd}
width={isStakeable ? BlockSize.Half : BlockSize.TwoThirds}
textAlign={TextAlign.End}
data-testid="multichain-token-list-item-secondary-value"
ellipsis={isStakeable}
>
{secondary}
</Text>
) : (
<Text
data-testid="multichain-token-list-item-value"
color={TextColor.textAlternative}
variant={TextVariant.bodyMd}
textAlign={TextAlign.End}
>
{primary} {isNativeCurrency ? '' : tokenSymbol}
</Text>
)}
<Text
data-testid="multichain-token-list-item-value"
color={TextColor.textAlternative}
variant={TextVariant.bodyMd}
textAlign={TextAlign.End}
>
{primary} {isNativeCurrency ? '' : tokenSymbol}
</Text>
</Box>
) : (
<Box
Expand Down Expand Up @@ -454,4 +453,8 @@ TokenListItem.propTypes = {
* address represents the token address
*/
address: PropTypes.string,
/**
* showPercentage represents if the increase decrease percentage will be hidden
*/
showPercentage: PropTypes.bool,
};
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ describe('TokenListItem', () => {
primary: '11.9751 ETH',
isNativeCurrency: true,
isOriginalTokenSymbol: false,
showPercentage: true,
};
const { getByTestId, getByText } = renderWithProvider(
<TokenListItem {...propsToUse} />,
Expand Down
12 changes: 5 additions & 7 deletions ui/pages/asset/components/__snapshots__/asset-page.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,12 @@ exports[`AssetPage should render a native asset 1`] = `
>
TEST
</span>
<div
class="mm-box mm-box--display-flex"
<p
class="mm-box mm-text mm-text--body-md mm-text--ellipsis mm-box--color-text-alternative"
data-testid="multichain-token-list-item-token-name"
>
<p
class="mm-box mm-text mm-text--body-md mm-text--font-weight-normal mm-text--ellipsis mm-box--color-text-default"
data-testid="token-increase-decrease-percentage-0x0000000000000000000000000000000000000000"
/>
</div>
TEST
</p>
</div>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-column mm-box--align-items-flex-end mm-box--width-2/3"
Expand Down

0 comments on commit 92ea1c9

Please sign in to comment.