-
Notifications
You must be signed in to change notification settings - Fork 5.4k
feat: virtualize the Tokens list #37589
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
Changes from all commits
694b49d
7a396f8
7657d32
55673e8
e0d6c20
62c5367
67ae764
446b78c
4cc29cf
7db0573
b14e98c
1bde564
a6a13fe
f81e585
d9986ec
58a9e1a
f7e0fc8
c994326
735622c
c9dfa11
c6411c3
3bbae99
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -377,6 +377,7 @@ | |
| "@sentry/utils": "^8.33.1", | ||
| "@solana/addresses": "2.0.0-rc.4", | ||
| "@swc/core": "^1.13.2", | ||
| "@tanstack/react-virtual": "^3.10.8", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very nice addition. I remember doing a PoC with the windowVirtualiser for our modals/popups with this (e.g. accounts list) |
||
| "@trezor/connect-web": "~9.6.0", | ||
| "@zxing/browser": "^0.1.5", | ||
| "@zxing/library": "0.21.3", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import React from 'react'; | ||
| import { | ||
| Display, | ||
| FontWeight, | ||
| TextVariant, | ||
| } from '../../../../../helpers/constants/design-system'; | ||
|
|
@@ -10,7 +9,6 @@ import { | |
| networkTitleOverrides, | ||
| } from '../../util/networkTitleOverrides'; | ||
| import { useI18nContext } from '../../../../../hooks/useI18nContext'; | ||
| import Tooltip from '../../../../ui/tooltip'; | ||
|
|
||
| type AssetCellTitleProps = { | ||
| title: string; | ||
|
|
@@ -19,27 +17,6 @@ type AssetCellTitleProps = { | |
| export const AssetCellTitle = ({ title }: AssetCellTitleProps) => { | ||
| const t = useI18nContext(); | ||
|
|
||
| if (title && title.length > 12) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are not using a tooltip for long titles anymore?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Can potentially use the title attribute for default browser experience)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are issues with this tooltip and the list. Morever, with the upcoming Side Panel mode where the Extension width can be adjusted to any size, basing it an arbitrary "length > 12" doesn't make sense. Truncation with ellipsis via CSS is more flexible. |
||
| return ( | ||
| <Tooltip | ||
| position="bottom" | ||
| html={title} | ||
| wrapperClassName="token-cell-title--ellipsis" | ||
| > | ||
| <Text | ||
| as="span" | ||
| data-testid="multichain-token-list-item-token-name" | ||
| fontWeight={FontWeight.Medium} | ||
| variant={TextVariant.bodyMd} | ||
| display={Display.Block} | ||
| ellipsis | ||
| > | ||
| {networkTitleOverrides(t as TranslateFunction, { title })} | ||
| </Text> | ||
| </Tooltip> | ||
| ); | ||
| } | ||
|
|
||
| // non-ellipsized title | ||
| return ( | ||
| <Text | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,9 @@ import React, { useContext, useEffect, useMemo } from 'react'; | |
| import { useSelector } from 'react-redux'; | ||
| import { type CaipChainId, type Hex } from '@metamask/utils'; | ||
| import { NON_EVM_TESTNET_IDS } from '@metamask/multichain-network-controller'; | ||
| import { useVirtualizer } from '@tanstack/react-virtual'; | ||
| import TokenCell from '../token-cell'; | ||
| import { ASSET_CELL_HEIGHT } from '../constants'; | ||
| import { | ||
| getEnabledNetworksByNamespace, | ||
| getIsMultichainAccountsState2Enabled, | ||
|
|
@@ -36,6 +38,7 @@ import { SafeChain } from '../../../../pages/settings/networks-tab/networks-form | |
| import { isGlobalNetworkSelectorRemoved } from '../../../../selectors/selectors'; | ||
| import { isEvmChainId } from '../../../../../shared/lib/asset-utils'; | ||
| import { sortAssetsWithPriority } from '../util/sortAssetsWithPriority'; | ||
| import { useScrollContainer } from '../../../../contexts/scroll-container'; | ||
|
|
||
| type TokenListProps = { | ||
| onTokenClick: (chainId: string, address: string) => void; | ||
|
|
@@ -45,6 +48,7 @@ type TokenListProps = { | |
| // TODO: Fix in https://github.com/MetaMask/metamask-extension/issues/31860 | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| function TokenList({ onTokenClick, safeChains }: TokenListProps) { | ||
| const scrollContainerRef = useScrollContainer(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| const isEvm = useSelector(getIsEvmMultichainNetworkSelected); | ||
| const newTokensImported = useSelector(getNewTokensImported); | ||
| const currentNetwork = useSelector(getSelectedMultichainNetworkConfiguration); | ||
|
|
@@ -145,6 +149,13 @@ function TokenList({ onTokenClick, safeChains }: TokenListProps) { | |
| allEnabledNetworksForAllNamespaces, | ||
| ]); | ||
|
|
||
| const virtualizer = useVirtualizer({ | ||
n3ps marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| count: sortedFilteredTokens.length, | ||
| getScrollElement: () => scrollContainerRef?.current || null, | ||
| estimateSize: () => ASSET_CELL_HEIGHT, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this fair with responsive design (increase/decrease window size/font size)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. virtualized.mov |
||
| overscan: 5, | ||
| }); | ||
n3ps marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| useEffect(() => { | ||
| if (sortedFilteredTokens) { | ||
| endTrace({ name: TraceName.AccountOverviewAssetListTab }); | ||
|
|
@@ -179,24 +190,39 @@ function TokenList({ onTokenClick, safeChains }: TokenListProps) { | |
| }); | ||
| }; | ||
|
|
||
| const virtualItems = virtualizer.getVirtualItems(); | ||
|
|
||
| return ( | ||
| <> | ||
| {sortedFilteredTokens.map((token: TokenWithFiatAmount) => { | ||
| <div | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we might want to reuse tanstack virtual for other areas, can we create a custom component that abstracts away some of these steps? const VirtualScrollView = (...) => {
// Step 2...
const virtualiser = useVirtualiser({ ... })
// Step 3...
return (
<div ...>
{/* Step 4... */}
virtualItems.map(virtualItem => {
return (
<div ...>
{renderItem({ item, virtualItem })}
</div>
)
})
</div>
)
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something I did a while ago (never merged)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eventually, we can abstract this once we get to see usage patterns from other tabs, so that the API and props match our needs. |
||
| className="relative w-full" | ||
| style={{ | ||
| height: `${virtualizer.getTotalSize()}px`, | ||
| }} | ||
| > | ||
| {virtualItems.map((virtualItem) => { | ||
| const token = sortedFilteredTokens[virtualItem.index]; | ||
| const isNonEvmTestnet = NON_EVM_TESTNET_IDS.includes( | ||
| token.chainId as CaipChainId, | ||
| ); | ||
|
|
||
| return ( | ||
| <TokenCell | ||
| <div | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| key={`${token.chainId}-${token.symbol}-${token.address}`} | ||
| token={token} | ||
| privacyMode={privacyMode} | ||
| onClick={isNonEvmTestnet ? undefined : handleTokenClick(token)} | ||
| safeChains={safeChains} | ||
| /> | ||
| className="absolute top-0 left-0 w-full" | ||
| style={{ | ||
| transform: `translateY(${virtualItem.start}px)`, | ||
| }} | ||
| > | ||
| <TokenCell | ||
| token={token} | ||
| privacyMode={privacyMode} | ||
| onClick={isNonEvmTestnet ? undefined : handleTokenClick(token)} | ||
| safeChains={safeChains} | ||
| /> | ||
| </div> | ||
| ); | ||
| })} | ||
| </> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import React, { createContext, useContext, useRef } from 'react'; | ||
|
|
||
| const ScrollContainerContext = | ||
| createContext<React.RefObject<HTMLDivElement> | null>(null); | ||
|
|
||
| /** | ||
| * Provides a ref to this container element for its child components | ||
| * | ||
| * @param props - HTML div attributes | ||
| * @param props.children - Child components to render inside the container | ||
| * @returns A div element with a ref accessible via useScrollContainer | ||
| */ | ||
| export const ScrollContainer = ({ | ||
| children, | ||
| ...props | ||
| }: React.HTMLAttributes<HTMLDivElement>) => { | ||
| const scrollRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| return ( | ||
| <ScrollContainerContext.Provider value={scrollRef}> | ||
| <div ref={scrollRef} {...props}> | ||
| {children} | ||
| </div> | ||
| </ScrollContainerContext.Provider> | ||
| ); | ||
| }; | ||
|
|
||
| /** | ||
| * Hook to access the scroll container ref from any child component | ||
| * | ||
| * @returns Ref to the scroll container | ||
| */ | ||
| export const useScrollContainer = () => { | ||
| return useContext(ScrollContainerContext); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import ConnectedSites from '../connected-sites'; | |
| import ConnectedAccounts from '../connected-accounts'; | ||
| import { isMv3ButOffscreenDocIsMissing } from '../../../shared/modules/mv3.utils'; | ||
| import ActionableMessage from '../../components/ui/actionable-message/actionable-message'; | ||
| import { ScrollContainer } from '../../contexts/scroll-container'; | ||
|
|
||
| import { | ||
| FontWeight, | ||
|
|
@@ -887,7 +888,7 @@ export default class Home extends PureComponent { | |
| !isSocialLoginFlow; | ||
|
|
||
| return ( | ||
| <div className="main-container main-container--has-shadow"> | ||
| <ScrollContainer className="main-container main-container--has-shadow"> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| <Route path={CONNECTED_ROUTE} component={ConnectedSites} exact /> | ||
| <Route | ||
| path={CONNECTED_ACCOUNTS_ROUTE} | ||
|
|
@@ -945,7 +946,7 @@ export default class Home extends PureComponent { | |
| </div> | ||
| {this.renderNotifications()} | ||
| </div> | ||
| </div> | ||
| </ScrollContainer> | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding virtualisation library. To investigate the
documentglobal property usage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From light inspection, seems to be used in the libraries examples. Uses
document.getElementById.https://github.com/search?q=repo%3ATanStack%2Fvirtual%20document.&type=code