-
Notifications
You must be signed in to change notification settings - Fork 10
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(MediaCard): add top icon support #824
Conversation
Size stats
|
Deploy preview for mistica-web ready! ✅ Preview Built with commit b1d3d00. |
Accessibility report ℹ️ You can run this locally by executing |
src/card.tsx
Outdated
@@ -407,6 +407,7 @@ interface MediaCardBaseProps { | |||
subtitle?: string; | |||
subtitleLinesMax?: number; | |||
description?: string; | |||
icon?: React.ReactElement |
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.
In figma it was called Asset, you may need to make this switch from icon to Asset
@FernandoRRosa
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, better call "asset" will be the new name for all props that contains assets. We want to make this breaking change in the next major and rename icon
to asset
to be more generic → #626
In my opinion, we have to embrace the new nomenclature and use asset
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.
hmmm... I think we should still use icon, as we do in other Card components. I agree with Yayo that we have a pending rename for this prop, but we should do that rename in all the cards at the same time. That rename will be done in a future mistica major release, not now
@@ -147,6 +147,22 @@ export const mediaCardContent = style([ | |||
}, | |||
]); | |||
|
|||
export const mediaCardIcon = style([ |
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.
If icon get changed to Asset will need to change here too.
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 add an asset
prop to playroom Media Card snippets?
mistica-web/playroom/snippets.tsx
Lines 608 to 646 in 45d96ce
group: 'Cards', | |
name: 'MediaCard with Image', | |
code: ` | |
<MediaCard | |
media={<Image src="https://picsum.photos/1200/1200" aspectRatio="16:9"/>} | |
headline={<Tag type="promo">Headline</Tag>} | |
pretitle="Pretitle" | |
title="Title" | |
subtitle="Subtitle" | |
description="Description" | |
extra={<Placeholder />} | |
button={ | |
<ButtonPrimary small onPress={() => {}}> | |
Action | |
</ButtonPrimary> | |
} | |
buttonLink={<ButtonLink onPress={() => {}}>Link</ButtonLink>} | |
/>`, | |
}, | |
{ | |
group: 'Cards', | |
name: 'MediaCard with Video', | |
code: ` | |
<MediaCard | |
media={<Video src="http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ElephantsDream.mp4" aspectRatio="16:9" />} | |
headline={<Tag color={colors.promo}>headline</Tag>} | |
pretitle="Pretitle" | |
title="Title" | |
subtitle="Subtitle" | |
description="Description" | |
extra={<Placeholder />} | |
button={ | |
<ButtonPrimary small onPress={() => {}}> | |
Action | |
</ButtonPrimary> | |
} | |
buttonLink={<ButtonLink onPress={() => {}}>Link</ButtonLink>} | |
/>`, | |
}, |
for example
asset={<Avatar size={40} src="https://source.unsplash.com/600x600/?face" />}
fcac9b7
to
b0a9a43
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.
nice 🤩
b0a9a43
to
f8e5a89
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.
what happened here?
Please review that extra bottom padding
src/card.tsx
Outdated
{asset} | ||
</Box> | ||
) : ( | ||
<Box paddingBottom={actions?.length || onClose ? 64 : 0} /> |
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.
This is not needed. It's needed in other cards like DisplayMediaCard because the asset
is not absolutely positioned like here.
src/card.css.ts
Outdated
paddingLeft: 24, | ||
paddingTop: 24, | ||
paddingBottom: 32, |
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.
@aweell, it's ok to have different paddings in desktop? I don't see this in the specs.
If these are needed, we should move them to the Box element instead of creating a new media query here @thiagoaraujo-tlf:
<Box paddingLeft={{mobile: 16, desktop: 24}} paddingTop={{mobile: 16, desktop: 24}}>
Also, I think paddingRight
and paddingBottom
are not needed, just left and top
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, the asset has a spacing left and top of 16 in mobile and 24 in desktop.
This is intended to align the asset to the content since the padding of the content increases in desktop.
The other changes, it seems they're related the content padding? They are already working in the current component.
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 just added icon padding for when it's on desktop @atabel
Screenshot tests report ✔️ All passing |
🎉 This PR is included in version 14.19.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Figma: https://www.figma.com/file/Y89JWviWrWV4RFFyqwCj3b/Asset-Media-Card---Circle-text?type=design&node-id=110-14234&mode=design&t=N44KdlRs1GH019sK-0