Skip to content

feat: MediaCard #242

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
Open
adrienzheng-cb wants to merge 1 commit into
base: adrien/card-refactor-base
Choose a base branch
from
Open

feat: MediaCard #242

adrienzheng-cb wants to merge 1 commit into from

Conversation

@adrienzheng-cb
Copy link
Contributor

@adrienzheng-cb adrienzheng-cb commented Dec 12, 2025 •
edited
Loading

What changed? Why?

Root cause (required for bugfixes)

UI changes

docs
storybook

Testing

How has it been tested?

  • Unit tests
  • Interaction tests
  • Pseudo State tests
  • Manual - Web
  • Manual - Android (Emulator / Device)
  • Manual - iOS (Emulator / Device)

Testing instructions

Illustrations/Icons Checklist

Required if this PR changes files under packages/illustrations/** or packages/icons/**

  • verified visreg changes with Terran (include link to visreg run/approval)
  • all illustration/icons names have been reviewed by Dom and/or Terran

Change management

type=routine
risk=low
impact=sev5

automerge=false

@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from ec9c499 to a6cb94a Compare December 15, 2025 14:58
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from a6cb94a to be85bd4 Compare December 15, 2025 19:30
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch 3 times, most recently from 830443f to e3d6651 Compare December 19, 2025 18:44
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from e3d6651 to 67a0cf5 Compare December 19, 2025 18:50
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from 67a0cf5 to 634007d Compare December 19, 2025 18:59
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/media-card branch 2 times, most recently from b8ba62a to a1c36fd Compare December 19, 2025 19:08
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from 634007d to d4869f5 Compare January 19, 2026 17:46
@adrienzheng-cb adrienzheng-cb marked this pull request as ready for review January 22, 2026 15:21
cb-ekuersch
cb-ekuersch reviewed Jan 26, 2026
View reviewed changes
packages/mobile/src/cards/MediaCard/index.tsx
export type MediaCardBaseProps = MediaCardLayoutProps;

export type MediaCardProps = Omit<CardRootProps, 'children'> &
MediaCardBaseProps & {
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

nit: full types before others

packages/mobile/src/cards/MediaCard/index.tsx

export type MediaCardBaseProps = MediaCardLayoutProps;

export type MediaCardProps = Omit<CardRootProps, 'children'> &
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

is there a reason you are composing the RootProps and not the RootBaseProps on the base props type? I trust your decision, I'm just trying to strengthen my own understanding of the distinction when I can. The benefit I see of doing it this way is you now make MediaCard polymorphic the same way as the CardRoot component

packages/mobile/src/cards/MediaCard/index.tsx
media,
mediaPlacement = 'end',
style,
styles: { root: rootStyle, ...layoutStyles } = {},
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

nice way to separate out the layout styles into their own component!

packages/mobile/src/cards/MediaCard/MediaCardLayout.tsx
import { Text } from '../../typography/Text';

export type MediaCardLayoutBaseProps = {
/** Text or React node to display as the card title. When a string is provided, it will be rendered in a CardTitle component. */
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

nit: CardTitle doesn't really exist from the customer's perspective. I would mention the font instead

packages/mobile/src/cards/MediaCard/MediaCardLayout.tsx
styles?: {
layoutContainer?: StyleProp<ViewStyle>;
contentContainer?: StyleProp<ViewStyle>;
textContainer?: StyleProp<ViewStyle>;
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

sometimes it feels unnecessary to have 2 levels deep of custom CSS - how does it feel to you? Imo they could always provide more targetted CSS or a ReactNode to customize further if needed (latter for mobile as no CSS there)

packages/web/src/cards/MediaCard/index.tsx
}
>;

export type MediaCardProps<AsComponent extends React.ElementType = 'article'> = Polymorphic.Props<
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

i commented on the wrong package's MediaCard - but touching on it here again. If you assign the MediaCard props to CardRootProps won't you get to share the polymorphism?

packages/web/src/cards/MediaCard/MediaCardLayout.tsx
import { Text } from '../../typography/Text';

export type MediaCardLayoutBaseProps = {
/** Text or React node to display as the card title. When a string is provided, it will be rendered in a CardTitle component. */
Copy link
Contributor

@cb-ekuersch cb-ekuersch Jan 26, 2026

Choose a reason for hiding this comment

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

nit: same thing about referencing components that don't exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cb-ekuersch cb-ekuersch cb-ekuersch left review comments

@hcopp hcopp Awaiting requested review from hcopp

@maximo-macchi-cb maximo-macchi-cb Awaiting requested review from maximo-macchi-cb

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@adrienzheng-cb @cb-ekuersch