Skip to content

feat: Atomic Components for Card refactor#235

Draft
adrienzheng-cb wants to merge 1 commit intomasterfrom
adrien/card-refactor-base
Draft

feat: Atomic Components for Card refactor#235
adrienzheng-cb wants to merge 1 commit intomasterfrom
adrien/card-refactor-base

Conversation

@adrienzheng-cb
Copy link
Contributor

What changed? Why?

Root cause (required for bugfixes)

UI changes

iOS Old iOS New
old screenshot new screenshot
Android Old Android New
old screenshot new screenshot
Web Old Web New
old screenshot new screenshot

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

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Dec 11, 2025

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 1
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1
CODEOWNERS 🟡 See below

🟡 CODEOWNERS

Code Owner Status Calculation
ui-systems-eng-team 🟡 0/1
Denominator calculation
Additional CODEOWNERS Requirement
Show calculation
Sum 0
0
From CODEOWNERS 1
Sum 1


export type CardRootProps = CardRootBaseProps & PressableProps;

export const CardRoot = memo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Component that handles the interactive layer of card

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrienzheng-cb do you mind adding some js docs to this (and other component s where helpful) to add context around usage?

Additionally, are we expecting customers to ever use this directly or is it for internal use to share behavior between Card components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment, I'm planning to just keep it internal along with all of the CardLayout component, only MediaCard, DataCard, MessagingCard, and ContentCard will be exposed. I do see benefit of exposing it if we encourage consumers to assemble their own card tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrienzheng-cb got it! In that case let's not export it from the barrel file nor add an exports entry for those modules in the package.json to make it harder for customers to use

@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from 4c49779 to ec9c499 Compare December 12, 2025 19:27
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch 2 times, most recently from a6cb94a to be85bd4 Compare December 15, 2025 19:30
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from be85bd4 to 7134a9f Compare December 19, 2025 18:26
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch from 7134a9f to 830443f Compare December 19, 2025 18:35
@adrienzheng-cb adrienzheng-cb force-pushed the adrien/card-refactor-base branch 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
Copy link
Contributor

@cb-ekuersch cb-ekuersch left a comment

Choose a reason for hiding this comment

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

Few questions I want to get to the bottom of here, but in general the direction looks good!


import { Pictogram, SpotSquare } from '../illustrations';

import { CardRemoteImage } from './CardRemoteImage';
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

<RemoteImage
alt={props.alt ?? ''}
resizeMode="cover"
source={getSource(props.src)}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: curious about the addition of these properties

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this properties used to be set inside CardRemoteImage which got removed.
resizeMode ensures that the Image always stretch proportionally covers its container entirely no matter what aspect ratio the container is in.


export type CardRootProps = CardRootBaseProps & PressableProps;

export const CardRoot = memo(
Copy link
Contributor

Choose a reason for hiding this comment

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

@adrienzheng-cb do you mind adding some js docs to this (and other component s where helpful) to add context around usage?

Additionally, are we expecting customers to ever use this directly or is it for internal use to share behavior between Card components?

{
children?: React.ReactNode;
/** If true, the CardRoot will be rendered as a Pressable component. */
renderAsPressable?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we just make this implicit with a onPress prop as the previous cards did?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know we discussed this like a month ago, i just need a little help remembering the context around the decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because the list of props that indicate is interactivity is indefinite, we just want to leave the choice to customer instead of having implicit logic here and there and make assumptions.

if (renderAsPressable) {
const { as, ...pressableRestProps } = props;
return (
<Pressable ref={ref} as={as ?? 'button'} {...(pressableRestProps as any)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is the default as necessary? Pretty sure button is the default for pressable

Copy link
Contributor

Choose a reason for hiding this comment

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

should we render the pressable if they pass button as the value for as? onClick will be valid prop at that point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice suggestion. I updated logic to the following in followup pr

  1. if as is button or a, then renderAsPressable is true by default, otherwise false.
  2. removed the default as, because Pressable by default renders a button.

} else {
const { as, ...hstackRestProps } = props;
return (
<HStack ref={ref} as={as ?? 'article'} {...(hstackRestProps as any)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why article?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been removed in follow-up. originally using article because semanticaly non-interactive card with a lot of text content should be rendered as an article. Since MediaCard, DataCard, and MessagingCard are intended to be more interactive than static, this is no longer necessary.

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

Development

Successfully merging this pull request may close these issues.

3 participants