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.

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(PosterCard): allow backgroundColor/variant #841

Merged
merged 12 commits into from
Aug 11, 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This card is impossible to have in dark mode. ThemeVariant is forcing these text to white :( I think that could be a problem. Can we force themeVariant also in dark mode?
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky because Text just uses textPrimary/textPrimaryInverse depending on the variant context, but those color constants are both white in dark mode. So the only way to fix this would be to find a way to disable dark mode in this area. We could explore a solution in a different PR to manage this case

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
yceballost marked this conversation as resolved.
Show resolved Hide resolved
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
39 changes: 39 additions & 0 deletions src/__screenshot_tests__/poster-card-screenshot-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,42 @@ test.each(TESTABLE_DEVICES)('PosterCard with asset in %s', async (device) => {

expect(image).toMatchImageSnapshot();
});

test.each(['inverse', 'alternative', 'default'])('PosterCard with variant %s', async (variant) => {
await openStoryPage({
id: 'components-cards-poster-card--default',
args: {variant, background: 'color from skin'},
});

const posterCard = await screen.findByTestId('main-poster-card');

const image = await posterCard.screenshot();

expect(image).toMatchImageSnapshot();
});

test('PosterCard with custom background color', async () => {
await openStoryPage({
id: 'components-cards-poster-card--default',
args: {background: 'custom color', backgroundColorCustom: '#ff0'},
});

const posterCard = await screen.findByTestId('main-poster-card');

const image = await posterCard.screenshot();

expect(image).toMatchImageSnapshot();
});

test('PosterCard with custom background color inverse', async () => {
await openStoryPage({
id: 'components-cards-poster-card--default',
args: {background: 'custom color', backgroundColorCustom: '#000', variant: 'inverse'},
});

const posterCard = await screen.findByTestId('main-poster-card');

const image = await posterCard.screenshot();

expect(image).toMatchImageSnapshot();
});
61 changes: 48 additions & 13 deletions src/__stories__/poster-card-story.tsx
atabel marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ const BACKGROUND_VIDEO_POSTER_SRC = beachImg;

type PosterCardArgs = {
asset: 'circle with icon' | 'circle with image' | 'none';
background: 'image' | 'video';
background: 'image' | 'video' | 'custom color' | 'color from skin';
backgroundColorCustom: string;
backgroundColorFromSkin: string;
variant: 'default' | 'inverse' | 'alternative';
headlineType: TagType;
headline: string;
pretitle: string;
Expand All @@ -47,6 +50,9 @@ type PosterCardArgs = {
export const Default: StoryComponent<PosterCardArgs> = ({
asset,
background,
backgroundColorCustom,
backgroundColorFromSkin,
variant,
headline,
headlineType,
pretitle,
Expand All @@ -70,24 +76,33 @@ export const Default: StoryComponent<PosterCardArgs> = ({
icon = <Circle size={40} backgroundImage={avatarImg} />;
}

const topActionsProps = {
onClose: closable ? () => {} : undefined,
actions: withTopAction
? [
{
Icon: IconLightningRegular,
onPress: () => {},
label: 'Lightning',
},
]
: undefined,
};
const backgroundProps =
background === 'image'
? {
onClose: closable ? () => {} : undefined,
actions: withTopAction
? [
{
Icon: IconLightningRegular,
onPress: () => {},
label: 'Lightning',
},
]
: undefined,
...topActionsProps,
backgroundImage: BACKGROUND_IMAGE_SRC,
}
: {
: background === 'video'
? {
backgroundVideo: BACKGROUND_VIDEO_SRC,
poster: BACKGROUND_VIDEO_POSTER_SRC,
}
: {
...topActionsProps,
backgroundColor: backgroundColorFromSkin || backgroundColorCustom,
variant,
};

const wrongBackgroundProps =
Expand All @@ -113,6 +128,7 @@ export const Default: StoryComponent<PosterCardArgs> = ({
return (
<Stack space={32} dataAttributes={{testid: 'poster-card'}}>
<PosterCard
dataAttributes={{testid: 'main-poster-card'}}
{...backgroundProps}
icon={icon}
headline={headline ? <Tag type={headlineType}>{headline}</Tag> : undefined}
Expand Down Expand Up @@ -168,6 +184,9 @@ Default.args = {
asset: 'none',
headlineType: 'promo',
background: 'image',
backgroundColorCustom: '',
backgroundColorFromSkin: '',
variant: 'default',
headline: 'Priority',
pretitle: 'Pretitle',
title: 'Title',
Expand All @@ -179,6 +198,7 @@ Default.args = {
height: 'auto',
aspectRatio: 'auto',
};

Default.argTypes = {
asset: {
options: ['circle with icon', 'circle with image', 'none'],
Expand All @@ -189,8 +209,23 @@ Default.argTypes = {
control: {type: 'select'},
},
background: {
options: ['image', 'video'],
options: ['image', 'video', 'color from skin', 'custom color'],
control: {type: 'select'},
},
backgroundColorCustom: {
control: {type: 'color'},
if: {arg: 'background', eq: 'custom color'},
},
backgroundColorFromSkin: {
control: {type: 'select'},
options: {'none (determined by variant)': '', ...skinVars.colors},
if: {arg: 'background', eq: 'color from skin'},
},
variant: {
options: ['default', 'inverse', 'alternative'],
control: {type: 'select'},
// this control should only be visible when background is set to 'color from skin' or 'custom color'
// if: {arg: 'background', eq: 'color'},
},
aspectRatio: {
options: ['1:1', '16:9', '7:10', '9:10', 'auto'],
Expand Down
13 changes: 12 additions & 1 deletion src/__type_tests__/card-type-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ import Image from '../image';
onPress={() => {}}
trackingEvent={{name: 'do-something'}}
/>;
<PosterCard title="title" isInverse />;
<PosterCard title="title" variant="inverse" />;
<PosterCard title="title" variant="alternative" />;
<PosterCard title="title" isInverse backgroundColor="red" />;
<PosterCard title="title" variant="inverse" backgroundColor="red" />;
Comment on lines +34 to +38
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are valid usage examples


// @ts-expect-error onPress and href can't be used together
<PosterCard title="title" onPress={() => {}} href="/" />;
Expand All @@ -42,8 +47,14 @@ import Image from '../image';
<PosterCard title="title" trackingEvent={{name: 'do-something'}} />;
// @ts-expect-error backgroundImage and backgroundVideo can't be used together
<PosterCard title="title" backgroundImage="" backgroundVideo="" />;
// @ts-expect-error backgroundImage or backgroundVideo are mandatory
// @ts-expect-error backgroundImage and backgroundColor can't be used together
<PosterCard title="title" backgroundImage="" backgroundColor="" />;
// @ts-expect-error backgroundVideo and backgroundColor can't be used together
<PosterCard title="title" backgroundVideo="" backgroundColor="" />;
// @ts-expect-error backtroundColor, backgroundImage or backgroundVideo are mandatory
atabel marked this conversation as resolved.
Show resolved Hide resolved
<PosterCard title="title" />;
// @ts-expect-error if you set a custom backgroundColor, you should specify the variant
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using a default value for the variant if it's not provided?

Copy link
Collaborator Author

@atabel atabel Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer this to be explicit. You set either backgroundImage, backgroundColor or backgroundVideo. backgroundColor can be set explicitly with a custom color (backgroundColor="red") or implicitly using an specific variant (variant="inverse" or isInverse)

<PosterCard title="title" backgroundColor="red" />;

(isTouchable: boolean) => <SnapCard title="title" href={isTouchable ? '/' : undefined} />;
(isTouchable: boolean) => <SnapCard title="title" to={isTouchable ? '/' : undefined} />;
Expand Down
16 changes: 0 additions & 16 deletions src/card.css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,22 +279,6 @@ export const snapCardTouchableHover = style([
},
]);

export const snapCardTouchableHoverTransparent = style([
snapCardTouchableBase,
{
display: 'flex',
height: '100%',

'@media': {
[mq.supportsHover]: {
':hover': {
backgroundColor: 'transparent',
},
},
},
},
]);

export const displayCardContainer = sprinkles({
width: '100%',
height: '100%',
Expand Down
57 changes: 52 additions & 5 deletions src/card.tsx
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment that @aweell said in the other PR. When the card doesn't have image or video and inverse is false, the hover should be the same or similar to row list hover or the rest of white background component hovers.

https://www.figma.com/file/tKdPOfcUALzVIh5oizFbm7/%F0%9F%94%B8-Cards-Specs?type=design&node-id=2094-7031&mode=design&t=UXAJ8MkmcdI5ik8a-4

@aweell maybe it is not too clear in specs, two teams overlooked it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fixed now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The background now work fine, but the top actions states also need to change depending if the background is media or no media variant="inverse", no media variant="default/alternative".

Screenshot 2023-08-11 at 11 19 48

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {assignInlineVars} from '@vanilla-extract/dynamic';
import Inline from './inline';
import {getPrefixedDataAttributes} from './utils/dom';

import type {Variant} from './theme-variant-context';
import type {PressHandler} from './touchable';
import type {VideoElement, VideoSource} from './video';
import type {ButtonLink, ButtonPrimary, ButtonSecondary} from './button';
Expand Down Expand Up @@ -1196,8 +1197,19 @@ type PosterCardWithVideoProps = Omit<PosterCardBaseProps, 'actions' | 'onClose'>
backgroundVideoRef?: React.RefObject<VideoElement>;
};

type PosterCardWithBackgroundColorProps = PosterCardBaseProps & {
backgroundColor?: string;
} & ExclusifyUnion<
| {
variant: Variant;
}
| {
isInverse: boolean;
}
>;

type PosterCardProps = MaybeTouchableCard<
ExclusifyUnion<PosterCardWithImageProps | PosterCardWithVideoProps>
ExclusifyUnion<PosterCardWithImageProps | PosterCardWithVideoProps | PosterCardWithBackgroundColorProps>
>;

const POSTER_CARD_MIN_WIDTH = 140;
Expand All @@ -1223,6 +1235,9 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>(
titleLinesMax,
description,
descriptionLinesMax,
variant,
isInverse,
backgroundColor,
...touchableProps
},
ref
Expand All @@ -1241,6 +1256,28 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>(
const {textPresets} = useTheme();

const isTouchable = touchableProps.href || touchableProps.to || touchableProps.onPress;
const normalizedVariant = variant || (isInverse ? 'inverse' : 'default');

const calcBackgroundColor = () => {
if (backgroundColor) {
return backgroundColor;
}

return {
default: vars.colors.backgroundContainer,
inverse: isExternalInverse
? vars.colors.backgroundContainerBrandOverInverse
: vars.colors.backgroundBrand,
alternative: vars.colors.backgroundAlternative,
}[normalizedVariant];
};

const overlayStyle =
backgroundImage || backgroundVideo
? styles.touchableCardOverlayMedia
: normalizedVariant === 'inverse'
? styles.touchableCardOverlayInverse
: styles.touchableCardOverlay;

return (
<CardContainer
Expand All @@ -1258,13 +1295,13 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>(
className={styles.boxed}
width="100%"
minHeight="100%"
isInverse
isInverse={!!backgroundImage || !!backgroundVideo || normalizedVariant === 'inverse'}
background={
backgroundImage || backgroundVideo
? isExternalInverse
? vars.colors.backgroundContainerBrandOverInverse
: vars.colors.backgroundContainer
: undefined
: calcBackgroundColor()
}
>
<BaseTouchable
Expand All @@ -1273,7 +1310,7 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>(
className={styles.touchable}
aria-label={ariaLabel}
>
{isTouchable && <div className={styles.touchableCardOverlayMedia} />}
{isTouchable && <div className={overlayStyle} />}

<div className={styles.displayCardContainer}>
<ThemeVariant isInverse={isExternalInverse}>
Expand Down Expand Up @@ -1366,7 +1403,17 @@ export const PosterCard = React.forwardRef<HTMLDivElement, PosterCardProps>(
</div>
</BaseTouchable>
</InternalBoxed>
<CardActionsGroup onClose={onClose} actions={actions} type="media" />
<CardActionsGroup
onClose={onClose}
actions={actions}
type={
!!backgroundImage || !!backgroundVideo
? 'media'
: normalizedVariant === 'inverse'
? 'inverse'
: 'default'
}
/>
</CardContainer>
);
}
Expand Down
Loading