RadioCard component #16619
RadioCard component #16619
Conversation
enjojoy
force-pushed
the
feat/radiocard
branch
from
5045312 to
9708497
Compare
|
🚀 Expo preview is ready!
|
enjojoy
force-pushed
the
feat/radiocard
branch
from
092b6b0 to
92b8ff7
Compare
|
@peter-sanderson here is the component |
enjojoy
force-pushed
the
feat/radiocard
branch
from
92b8ff7 to
cbc8111
Compare
There was a problem hiding this comment.
Lets investigate if we can reuse ComponentWithSubIcon
Otherwise fine by me 👍
But please, lets wait for review from @jvaclavik / @adamhavel as well
I wouldn't use it because it has a border and also it is not placed on the border of the component but outside of it. It's up to @trezor/suite-engagement if they would like to remake it to use it in radio card as well 
|
enjojoy
force-pushed
the
feat/radiocard
branch
from
cbc8111 to
1cc4fea
Compare
enjojoy
force-pushed
the
feat/radiocard
branch
from
1cc4fea to
b4a4309
Compare
| z-index: 2; | ||
| border-radius: ${borders.radii.full}; | ||
| background-color: ${({ theme }) => theme.borderSecondary}; | ||
| height: 14px; |
There was a problem hiding this comment.
I'd rather not set dimensions like this. Just set the right icon size and only use padding here. Then you won't need the flex properties below.
| `; | ||
|
|
||
| const IconWrapper = styled.div` | ||
| position: absolute; |
There was a problem hiding this comment.
You can extract these position and z-index props into a separate Box component
| `; | ||
|
|
||
| export const RadioCard = ({ isActive, children }: RadioCardProps) => ( | ||
| <Box position={{ type: 'relative' }} width="100%" height="100%"> |
There was a problem hiding this comment.
I don't think the dimensions are not needed?
There was a problem hiding this comment.
Then I guess I'll add dimensions as an optional prop? They're needed here for sure
There was a problem hiding this comment.
I still don't get it:( Why w
Do you need them in this context? Width is 100% by default, or just use Column with stretch align. And height is dynamic based on content anyway?
There was a problem hiding this comment.
I think this makes sense, didn't think of the stretch. Thank you
enjojoy
force-pushed
the
feat/radiocard
branch
from
b4a4309 to
62d9e2e
Compare
suite-common/icons/src/icons.ts
Outdated
| @@ -635,6 +635,7 @@ export const icons = { | |||
| checkFat: require('../assets/checkFat.svg'), | |||
| checkFatFilled: require('../assets/checkFatFilled.svg'), | |||
| checkFilled: require('../assets/checkFilled.svg'), | |||
| checkMedium: require('../assets/checkMedium.svg'), | |||
There was a problem hiding this comment.
Can you please use checkCircleFilled or ComponentWithSubIcon
(https://dev.suite.sldev.cz/components/develop/?path=/story/componentwithsubicon--component-with-sub-icon)? 🙏
We don't want to add new icons, because if we get new full export from designers, we might replace all the custom added icons by accident.
There was a problem hiding this comment.
I tried them all before but they're just not as good as the one on figma... I will discuss it with the designer
There was a problem hiding this comment.
Will use the existing thin checkmark as agreed with designer
| padding: 2px; | ||
| `; | ||
|
|
||
| export const RadioCard = ({ isActive, children }: RadioCardProps) => ( |
There was a problem hiding this comment.
nitpick: It would be nice to add also frameProps, but we can do it later on our own.
| outline: ${borders.widths.large} solid ${({ theme }) => theme.borderSecondary}; | ||
| outline-offset: -${borders.widths.large}; |
There was a problem hiding this comment.
It is OK to use outline? IMHO yes.
@adamhavel What's your opinion here?
| outline-offset: -${borders.widths.large}; | ||
| `} | ||
| border-radius: ${borders.radii.md}; | ||
| z-index: 1; |
There was a problem hiding this comment.
This is not necessary if you move BorderActive below Card:
<Card paddingType="small" fillType="none">
{children}
</Card>
<BorderActive isActive={isActive} />There was a problem hiding this comment.
Thanks, it's a nice catch
| const IconWrapper = styled.div` | ||
| border-radius: ${borders.radii.full}; | ||
| background-color: ${({ theme }) => theme.borderSecondary}; | ||
| padding: 2px; |
There was a problem hiding this comment.
Could you use spacingsPx.xxxs? 🙏
enjojoy
force-pushed
the
feat/radiocard
branch
from
62d9e2e to
be152e0
Compare
enjojoy
force-pushed
the
feat/radiocard
branch
from
be152e0 to
6d0c5b9
Compare
|
I noticed a leftower type that is not needed anymore, will add a fix now |
|
QA OK Info:
|
Description
New RadioCard component, used in fees
Screenshots: