-
-
Notifications
You must be signed in to change notification settings - Fork 325
Feat/22838 asset picker input in trading (swap & buy) #23314
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
base: develop
Are you sure you want to change the base?
Changes from all commits
ee56595
12e196f
c4524a9
0e35f59
f235b78
ce36d6d
e763c38
f2c4a6d
431e05b
b3a088e
122ff63
a4b9d57
4354de0
ff87318
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 |
|---|---|---|
|
|
@@ -13,12 +13,12 @@ import { ItemClickableContainer } from '../ItemClickableContainer'; | |
|
|
||
| export type AssetRowReceiveToAccountProps = { | ||
| account: Account; | ||
| 'data-testid'?: string; | ||
| dataTestId?: string; | ||
|
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. Same here, no need to change naming here
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. Because then you can't just pass it to the component but you need to re-assigned it to another variable (with valid name). So this has valid name from the beginning. Nothing wrong about that. This gonna stay.
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. Sorry but I disagree.
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. I initially hoped we can focus during review on important stuff. And you somewhat did, for example with the perf. issue and some imperfect styles thanks for that. But this is not one them and we are losing time by discussing something that not important at all.
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. I totally understand that a lot of stuff are not that important. And I'm also fine with approving PR without fixing all of them. But at the same time, we follow some rules and good practises for a good reason. Like not using classnames, or be very strict with unified prop names. I understand you are used to do it in different way, but we set these rules based on our previous experience and so far we have very good results with them. Please respect that some of these rules are there and we can be pretty irritating with enforcing them. If you follow them I'm sure you will get our β much faster. Also we care about the visual language and we want to have it as consistent as possible. Also if you want to ship features faster, I suggest to create small PRs. It's extremely hard to understand logic in PRs with 1000+ lines, it takes too much time to review and I understand it can be also pretty hard to merge it.
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. I don't remember last time I did so huge PR and I didn't initially expect it to grow that much. I'm sorry about that I know it's then hard to review it properly. As I wrote today to the PR comment and created all commits again in much more granular fashion and categorized them by prio. in the PR description. Hopefully, it will help a bit. |
||
| onClick: (account: Account) => void; | ||
| }; | ||
|
|
||
| export function AssetRowReceiveToAccount({ | ||
| 'data-testid': dataTestId, | ||
| dataTestId, | ||
| account, | ||
| onClick, | ||
| }: AssetRowReceiveToAccountProps) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| export * from './constants'; | ||
| export * from './AssetRowReceiveToAccount'; | ||
| export * from './AssetRowSendFromAccount'; | ||
| export * from './AssetRowAccountWithBalance'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import { TradingAssetOption } from '@suite-common/trading'; | ||
| import { Row } from '@trezor/components'; | ||
| import { AssetLogo, CoinLogo, shouldShowNetworkIcon } from '@trezor/product-components'; | ||
| import { spacings } from '@trezor/theme'; | ||
|
|
||
| import { AssetDetails } from '../AssetDetails'; | ||
| import { ItemClickableContainer } from '../ItemClickableContainer'; | ||
|
|
||
| export const ASSET_ROW_ASSET_HEIGHT = 68; | ||
|
|
||
| export type AssetRowAssetProps = { | ||
| asset: TradingAssetOption; | ||
| onClick: (asset: TradingAssetOption) => void; | ||
| dataTestId?: string; | ||
cermakjiri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }; | ||
|
|
||
| export function AssetRowAsset({ asset, dataTestId, onClick }: AssetRowAssetProps) { | ||
| return ( | ||
| <ItemClickableContainer | ||
| onClick={() => { | ||
| onClick(asset); | ||
| }} | ||
| > | ||
| <Row data-testid={`${dataTestId}/${asset.id}`} gap={spacings.sm}> | ||
| {asset.isNativeToken ? ( | ||
| <CoinLogo size={40} symbol={asset.symbol} type="tokenWithNetwork" /> | ||
| ) : ( | ||
| <AssetLogo | ||
| size={40} | ||
| coingeckoId={asset.coingeckoId} | ||
| symbol={asset.networkSymbol} | ||
| contractAddress={asset.contractAddress} | ||
| placeholder={asset.displaySymbol} | ||
| showNetworkIcon={shouldShowNetworkIcon( | ||
| asset.networkSymbol, | ||
| asset.contractAddress, | ||
| )} | ||
| /> | ||
| )} | ||
| <AssetDetails | ||
| name={asset.name} | ||
| symbol={asset.symbol} | ||
| networkName={asset.networkName} | ||
| /> | ||
| </Row> | ||
| </ItemClickableContainer> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,43 +2,39 @@ import { getCoingeckoId, getDisplaySymbol } from '@suite-common/wallet-config'; | |
| import { Account } from '@suite-common/wallet-types'; | ||
| import { asBaseCurrencyAmount } from '@suite-common/wallet-utils'; | ||
| import { Row } from '@trezor/components'; | ||
| import { AssetLogo } from '@trezor/product-components'; | ||
| import { AssetLogo, shouldShowNetworkIcon } from '@trezor/product-components'; | ||
| import { spacings } from '@trezor/theme'; | ||
|
|
||
| import { TokensWithRates } from 'src/utils/wallet/tokenUtils'; | ||
|
|
||
| import { ItemClickableContainer } from '../ItemClickableContainer'; | ||
| import { AssetAmount } from './AssetAmount'; | ||
| import { AssetDetails } from './AssetDetails'; | ||
| import { AssetDetails } from '../AssetDetails'; | ||
|
|
||
| export const ASSET_ROW_TOKEN_HEIGHT = 68; | ||
|
|
||
| export type AssetRowTokenProps = { | ||
| token: TokensWithRates; | ||
| account: Account; | ||
| onClick: (token: TokensWithRates, account: Account) => void; | ||
| 'data-testid'?: string; | ||
| dataTestId?: string; | ||
|
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. Same here, no need to change naming here
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. |
||
| }; | ||
|
|
||
| export function AssetRowToken({ | ||
| token, | ||
| account, | ||
| 'data-testid': dataTestId, | ||
| onClick, | ||
| }: AssetRowTokenProps) { | ||
| export function AssetRowToken({ token, account, dataTestId, onClick }: AssetRowTokenProps) { | ||
| return ( | ||
| <ItemClickableContainer | ||
| onClick={() => { | ||
| onClick(token, account); | ||
| }} | ||
| > | ||
| <Row data-testid={dataTestId} gap={spacings.sm}> | ||
| <Row data-testid={`${dataTestId}/${account.symbol}/${token.symbol}`} gap={spacings.sm}> | ||
| <AssetLogo | ||
| size={40} | ||
| coingeckoId={getCoingeckoId(account.symbol) ?? account.networkType} | ||
| coingeckoId={getCoingeckoId(account.symbol)!} | ||
|
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. I'm not happy about this. We should have better typings or use some early return (my preferred variant here) or fallback value. Overusing of
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. If you take closer look on
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. I'm not questioning your typing improvements. I just want to say that I believe it should be pretty easy to get rid of
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. The |
||
| symbol={account.symbol} | ||
| contractAddress={token.contract} | ||
| placeholder={getDisplaySymbol(token.symbol!, token.contract)} | ||
| showNetworkIcon={shouldShowNetworkIcon(account.symbol, token.contract)} | ||
| /> | ||
| <AssetDetails | ||
| name={token.name!} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,8 @@ | ||
| export * from './AssetsList/AssetsList'; | ||
| export * from './AssetsListEmpty/AssetsListEmpty'; | ||
| export * from './AssetSearchWithNetworkFilter/AssetSearchWithNetworkFilter'; | ||
| export * from './AssetsModal/AssetsModal'; | ||
| export * from './AssetRow/AssetRowAccount'; | ||
| export * from './AssetRow/AssetGroupLabel'; | ||
| export * from './AssetRow/AssetGroupSpace'; | ||
| export * from './AssetRow/AssetRowToken/AssetRowToken'; | ||
| export * from './AssetRow/AssetRowAsset/AssetRowAsset'; | ||
cermakjiri marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.
What is the reason for this change? We use this prop name only in 4 files and I would prefer to keep the same and consistent naming.