Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(MediaCard): add top icon support #824

Merged
merged 12 commits into from
Jul 28, 2023
Merged

feat(MediaCard): add top icon support #824

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ce14917
feat(PTIECBC03-4665): media-card circle
thiagoaraujo-tlf Jul 18, 2023
6b3511d
fix: switch name from icon to asset
gabrielmarquesini-tlf Jul 19, 2023
ebbb6ac
fix: prettier executed in component story
gabrielmarquesini-tlf Jul 19, 2023
f8e5a89
feat: asset props in media card playroom
gabrielmarquesini-tlf Jul 20, 2023
518abbd
fix: resolving conflicting files
gabrielmarquesini-tlf Jul 20, 2023
c34aa8d
Merge branch 'master' into media-card-circle-component
gabrielmarquesini-tlf Jul 21, 2023
fb2824c
Merge branch 'master' into media-card-circle-component
gabrielmarquesini-tlf Jul 24, 2023
80be765
fix: switch name from asset to icon
gabrielmarquesini-tlf Jul 24, 2023
f5498ff
fix: media card padding when closing
gabrielmarquesini-tlf Jul 24, 2023
02fa71b
fix: media card screenshot test
gabrielmarquesini-tlf Jul 24, 2023
1917d9b
Merge branch 'master' into media-card-circle-component
gabrielmarquesini-tlf Jul 24, 2023
b1d3d00
fix: media card icon padding on desktop
gabrielmarquesini-tlf Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified .../__image_snapshots__/media-card-screenshot-test-tsx-media-card-group-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...hots__/media-card-screenshot-test-tsx-media-card-with-body-closeable-1-snap.png
View file
Open in desktop
Copy link
Collaborator

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

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...apshots__/media-card-screenshot-test-tsx-media-card-with-top-actions-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
22 changes: 22 additions & 0 deletions src/__stories__/media-card-story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ import {
Image,
Tag,
IconMobileDeviceRegular,
Circle,
skinVars,
} from '..';
import ResponsiveLayout from '../responsive-layout';
import {Placeholder} from '../placeholder';
import tennisImg from './images/tennis.jpg';
import confettiVideo from './videos/confetti.mp4';
import avatarImg from './images/avatar.jpg';

import type {TagType} from '..';

Expand All @@ -26,6 +29,7 @@ const VIDEO_SRC = confettiVideo;
const IMAGE_SRC = tennisImg;

type Args = {
asset: 'icon' | 'circle + icon' | 'image' | 'circle + image';
media: 'image' | 'video';
headlineType: TagType;
headline: string;
Expand All @@ -51,7 +55,19 @@ export const Default: StoryComponent<Args> = ({
closable,
withTopAction,
media,
asset,
}) => {
let icon;
if (asset === 'circle + icon') {
icon = (
<Circle size={40} backgroundColor={skinVars.colors.brandLow}>
<IconMobileDeviceRegular color={skinVars.colors.brand} />
</Circle>
);
} else if (asset === 'circle + image') {
icon = <Circle size={40} backgroundImage={avatarImg} />;
}

const button = actions.includes('button') ? (
<ButtonPrimary small href="https://google.com">
Action
Expand Down Expand Up @@ -79,6 +95,7 @@ export const Default: StoryComponent<Args> = ({
title={title}
subtitle={subtitle}
description={description}
asset={icon}
media={
media === 'video' ? (
<Video src={VIDEO_SRC} aspectRatio="16:9" dataAttributes={{qsysid: 'video'}} />
Expand Down Expand Up @@ -108,6 +125,7 @@ export const Default: StoryComponent<Args> = ({

Default.storyName = 'Media card';
Default.args = {
asset: 'icon',
media: 'image',
headlineType: 'promo',
headline: 'Priority',
Expand All @@ -121,6 +139,10 @@ Default.args = {
withTopAction: false,
};
Default.argTypes = {
asset: {
options: ['circle + icon', 'circle + image', 'none'],
control: {type: 'select'},
},
media: {
options: ['image', 'video'],
control: {type: 'select'},
Expand Down
16 changes: 16 additions & 0 deletions src/card.css.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,22 @@ export const mediaCardContent = style([
},
]);

export const mediaCardAsset = style([
{
position: 'absolute',
zIndex: 1,
},
{
'@media': {
[mq.desktopOrBigger]: {
paddingLeft: 24,
paddingTop: 24,
paddingBottom: 32,
Copy link
Collaborator

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

Copy link
Contributor

@aweell aweell Jul 26, 2023 •
edited
Loading

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.

Mobile:
Screenshot 2023-07-26 at 17 02 43

Desktop:
Screenshot 2023-07-26 at 17 02 46

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.

Mobile:
Screenshot 2023-07-26 at 17 06 17

Desktop:
Screenshot 2023-07-26 at 17 05 19

Copy link
Contributor

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

},
},
},
]);

export const dataCard = style([
sprinkles({
display: 'flex',
Expand Down
9 changes: 9 additions & 0 deletions src/card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,7 @@ interface MediaCardBaseProps {
subtitle?: string;
subtitleLinesMax?: number;
description?: string;
asset?: React.ReactElement;
descriptionLinesMax?: number;
extra?: React.ReactNode;
actions?: Array<CardAction>;
Expand Down Expand Up @@ -437,6 +438,7 @@ export const MediaCard = React.forwardRef<HTMLDivElement, MediaCardProps>(
title,
titleLinesMax,
description,
asset,
descriptionLinesMax,
extra,
actions,
Expand Down Expand Up @@ -486,6 +488,13 @@ export const MediaCard = React.forwardRef<HTMLDivElement, MediaCardProps>(
buttonLink={buttonLink}
/>
</div>
{asset ? (
<Box className={styles.mediaCardAsset} paddingX={16} paddingY={16}>
{asset}
</Box>
) : (
<Box paddingBottom={actions?.length || onClose ? 64 : 0} />
Copy link
Collaborator

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.

)}
</div>
</BaseTouchable>
</Boxed>
Expand Down