-
-
Notifications
You must be signed in to change notification settings - Fork 251
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/coinmarket crypto select #14551
Conversation
bab3e17
to
17c25c9
Compare
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.
@tomasklim 2. & 3. are fixed. 1. Let's add it in the future. 🚀 |
a: CoinmarketCryptoSelectItemProps, | ||
b: CoinmarketCryptoSelectItemProps, | ||
) => { | ||
const order = ['bitcoin', 'ethereum', 'litecoin', 'solana', 'cardano']; |
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.
based on?
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.
According to earlier sorting based on the backend's response. Now there is no sorting on the backend.
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.
I would sort it using order from networksConfig
.
cc @Hannsek maybe we should also sort it somehow?
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.
Good job Houmi 👏
const ColumnWrapper = styled.div<{ $height: string }>` | ||
display: flex; | ||
flex-direction: column; | ||
align-items: center; | ||
justify-content: center; | ||
height: ${({ $height }) => $height}; | ||
min-height: 180px; | ||
`; |
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.
Btw you could do this with Column
. We can add min-height there if you need.
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.
It is added in this commit e30ec34. We can also create separate PR if necessary.
|
||
const ParagraphWrapper = styled.div` | ||
max-width: 280px; | ||
margin: 0 auto; |
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 about combine these margins?
margin: ${spacingsPx.xxxs} auto 0 auto;
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.
Done
min-height: 180px; | ||
`; | ||
|
||
const ParagraphWrapper = styled.div` |
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.
Not 100% sure but you might use Paragraph
component instead of Text
+ ParagraphWrapper
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.
Nice improvement! Done
}; | ||
|
||
// temporary styles, should be replaced in the future | ||
const CheckableTag = styled.button<CheckableTagProps>` |
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.
Could you please extract this component and related logic to different file? 🙏 we will later refactor it to the design system
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.
It is done
import { FormattedMessage } from 'react-intl'; | ||
|
||
interface CheckableTagProps { | ||
$variant: 'primary' | 'tertiary'; |
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.
Could you use it this way please? 🙏
export const tagVariants = ['primary', 'tertiary'] as const;
export type TagVariant = Extract<UIVariant, (typeof tagVariants)[number]>;
we use similar approach everywhere we use variants
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.
Done! Thanks
onSelectAssetModal: (selectedAsset: string) => void; | ||
onFavoriteClick: (isFavorite: boolean) => void; | ||
}; | ||
const LIST_HEIGHT = 'calc(80vh - 267px)'; // 267px is height header part of modal |
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.
Could we do it without comment please? 👍
Suggestion:
const HEADER_HEIGHT = 267
const LIST_HEIGHT = 'calc(80vh - ${HEADER_HEIGHT}px)';
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.
Done!
onFavoriteClick: (isFavorite: boolean) => void; | ||
}; | ||
const LIST_HEIGHT = 'calc(80vh - 267px)'; // 267px is height header part of modal | ||
const height = 60; |
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.
I'm not sure what does this mean. Maybe it's item height?
In that case I suggest:
const ITEM_HEIGHT = 60
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.
Ye, nice catch. Updated
name={name} | ||
symbol={symbol} | ||
isFavorite={isFavorite} | ||
badge={badge} | ||
logo={{ |
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.
nit: thinking that it would be more clear this way:
logo={<AssetLogo ... />}
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.
Done
label: string; // token shortcut | ||
cryptoName: string | undefined; // full name |
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.
Please try to do it with self-explanatory props 🙏
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.
Same as comment above
#14551 (comment)
@@ -113,7 +131,7 @@ export const CoinmarketFormInputCryptoSelect = < | |||
}} | |||
data-testid="@coinmarket/form/select-crypto" | |||
isClearable={false} | |||
isSearchable | |||
menuIsOpen={false} |
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.
maybe could you please rename it toisMenuOpen
? 🙏
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.
Nice improvement. I have renamed it, but it is trezor component as well. I'm not sure it is related to this PR. Anyway it is in a separate commit - 0c30002
@jvaclavik thank you for constructive feedback. FYI:
|
packages/product-components/src/components/SelectAssetModal/SelectAssetModal.tsx
Show resolved
Hide resolved
|
||
const getNetworks = () => { | ||
const eth = getNetwork('eth'); | ||
const polygon = getNetwork('matic'); |
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.
There is #14093 which renames the matic to pol. Should we use pol here instead of matic?
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.
yes
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 purpose of getting here those 4 networks?
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.
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.
After rebase, I will change it. )
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.
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.
Next to All network
is not a random number. It is the number of all available networks. Soon it would be nice to add some select as you showed to us in DMs.
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.
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.
I finally understood, what you meant. :D Maybe we should change All networks
to something like All token networks
? What do you think?
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.
Please use order from networksConfig. We will sort it before freeze
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.
Mostly just product issues, I will check the code on Monday
...let/coinmarket/common/CoinmarketForm/CoinmarketFormInput/CoinmarketFormInputCryptoSelect.tsx
Show resolved
Hide resolved
@@ -44,13 +53,21 @@ type VirtualizedListProps<T> = { | |||
items: Array<T & { height: number }>; | |||
onScroll?: (e: Event) => void; | |||
onScrollEnd: () => void; | |||
height: number | string; | |||
listHeight: number | string; | |||
listMinHeight: number | string; | |||
renderItem: (item: T, index: number) => React.ReactNode; | |||
}; | |||
|
|||
export const VirtualizedList = forwardRef<HTMLDivElement, VirtualizedListProps<any>>( |
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.
- The shadow at the bottom is over scrollbar
- scrollbar should probably not be over items
Screen.Recording.2024-09-27.at.16.14.26.mov
cc @jvaclavik
|
||
const getNetworks = () => { | ||
const eth = getNetwork('eth'); | ||
const polygon = getNetwork('matic'); |
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.
a: CoinmarketCryptoSelectItemProps, | ||
b: CoinmarketCryptoSelectItemProps, | ||
) => { | ||
const order = ['bitcoin', 'ethereum', 'litecoin', 'solana', 'cardano']; |
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.
I would sort it using order from networksConfig
.
cc @Hannsek maybe we should also sort it somehow?
packages/product-components/src/components/SelectAssetModal/AssetItem.tsx
Show resolved
Hide resolved
@marek-andrsk |
packages/product-components/src/components/SelectAssetModal/SelectAssetModal.tsx
Outdated
Show resolved
Hide resolved
|
||
const getNetworks = () => { | ||
const eth = getNetwork('eth'); | ||
const polygon = getNetwork('matic'); |
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.
Please use order from networksConfig. We will sort it before freeze
packages/product-components/src/components/SelectAssetModal/SelectAssetModal.tsx
Outdated
Show resolved
Hide resolved
It is not possible to get offer for |
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.
LGTM 🎸
1e81d6a
to
efec9bf
Compare
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.
Resolve the conflict and good job :)
efec9bf
to
b1afdee
Compare
Description
Design
Before
In Buy section
In Exchange section
After
Related
Resolve #14550