Repository navigation
Conversation
|
|
||
| type CardVariantProps = FilledCardProps | ElevatedCardProps | OutlinedCardProps; | ||
|
|
||
| export type Props = Omit<ViewProps, 'children' | 'style'> & |
| const type = | ||
| (hasUnionDeclarations && new Set(declarationTypes).size > 1) || | ||
| !declarations.includes(property) | ||
| new Set(declarationTypes).size > 1 || !declarations.includes(property) |
There was a problem hiding this comment.
This changes prop types on every component page. Have you checked the others?
|
And we've got conflicts |
BREAKING CHANGE: Card.Content now always applies fixed padding, and Card.Actions no longer assigns button modes or child margins.
BREAKING CHANGE: Card now accepts media, header, content, and actions slots; arbitrary children composition and the mode prop are removed.
a2f4215 to
8318785
Compare
|
Review by Claude iOS
Android
Web
(via Claude, verify if it makes sense) |
There was a problem hiding this comment.
Review by Claude
Points without a diff line:
- The branch conflicts with main. #5076 landed and changed
Card.tsx,CardActions.tsx,CardContent.tsx,CardCover.tsx,utils.tsx, the Card tests andCardExample.tsx. The open thread onCard.tsxline 178 about slots versuschildrenneeds an answer before the rebase, because main now has the context-basedchildrenAPI. - The
Card.Contentpadding change is a visible migration change that the guide does not mention. In v5 the padding depended on neighbors. Now it is always 16 on all sides, soCard.Contentfollowed byCard.Actionsgets 24 between the text and the buttons. (via Claude, verify if it makes sense)
| contentStyle, | ||
| theme: themeOverrides, | ||
| testID, | ||
| testID = 'card', |
There was a problem hiding this comment.
The default testID is still here. The reply on the earlier thread says it was removed, but only the derived -container and -visual IDs went away. #5099 removed default test IDs because they collide. On the example screen, 5 neutral Cards expose the identifier card on iOS and the resource-id card on Android, so getByTestId('card') and E2E selectors match several nodes. The tests in Card.test.tsx that use getByTestId('card') need an explicit testID after this change.
| testID = 'card', | |
| testID, |
(via Claude, verify if it makes sense)
| const stateLayerAnimatedStyle = useAnimatedStyle( | ||
| () => ({ | ||
| opacity: currentInteractiveVisuals.value.stateLayerOpacity, | ||
| transitionDuration, | ||
| transitionProperty: ['opacity'], | ||
| }), | ||
| [currentInteractiveVisuals, transitionDuration] | ||
| ); | ||
| const outlineAnimatedStyle = useAnimatedStyle(() => { | ||
| const outlineColor = currentInteractiveVisuals.value.outlineColor; | ||
|
|
||
| const total = React.Children.count(children); | ||
| const siblings = React.Children.map(children, (child) => | ||
| React.isValidElement(child) && child.type | ||
| ? typeof child.type !== 'string' && 'displayName' in child.type | ||
| ? child.type.displayName | ||
| : null | ||
| : null | ||
| return { | ||
| borderColor: outlineColor, | ||
| opacity: currentInteractiveVisuals.value.outlineOpacity, | ||
| transitionDuration, | ||
| transitionProperty: | ||
| typeof outlineColor === 'string' | ||
| ? ['borderColor', 'opacity'] | ||
| : ['opacity'], | ||
| }; | ||
| }, [currentInteractiveVisuals, transitionDuration]); | ||
| const focusIndicatorAnimatedStyle = useAnimatedStyle( | ||
| () => ({ | ||
| opacity: disabledState.value ? 0 : focused.value ? 1 : 0, | ||
| transitionDuration, | ||
| transitionProperty: ['opacity'], | ||
| }), | ||
| [disabledState, transitionDuration] | ||
| ); |
There was a problem hiding this comment.
These worklets return transitionDuration and transitionProperty, but Reanimated CSS transitions run when React commits new style props. A value that a worklet writes on the UI thread does not start one. I recorded the iOS simulator at 60 fps while pressing the filled Card: the pressed layer appears and disappears in a single frame on press in and press out. So short3, the standard easing, theme.animation.scale and reduced motion have no visible effect on the state layer, the outline and the focus indicator. AnimatedElevationSurface in Surface.tsx has the same shape, so elevation changes also jump.
Since these are changes between fixed states, use React state with Reanimated CSS transitions instead. The comment on line 487 shows how.
The snapshot tests do not catch this because they only assert the style objects. (via Claude, verify if it makes sense)
| <TouchableRipple | ||
| {...actionableAccessibilityProps} | ||
| ref={touchableRef} | ||
| testID={testID} | ||
| borderless={false} | ||
| hoverColor="transparent" |
There was a problem hiding this comment.
Pressed feedback is drawn twice. TouchableRipple adds its own pressed feedback (the underlay on iOS, the ripple on Android, both stateLayerPressed, which is onSurface at 10%), and the Card state layer adds another 10% onSurface for pressed. On the iOS simulator in the dark theme, the filled Card goes from gray value 51 to 86 while pressed. One 10% layer gives about 68.
hoverColor="transparent" already handles this for hover on web. For press, let one of the two own it. Keeping the ripple is closer to Material on Android, so set the Card pressed state layer opacity to 0 and keep the pressed elevation and outline tokens. The other option is rippleColor and underlayColor set to transparent, which removes the Android ripple. (via Claude, verify if it makes sense)
| <Animated.View | ||
| pointerEvents="none" | ||
| style={[ | ||
| StyleSheet.absoluteFill, | ||
| shapeStyle, | ||
| { | ||
| backgroundColor: visuals.stateLayerColor, | ||
| }, | ||
| stateLayerTransitionStyle, | ||
| stateLayerAnimatedStyle, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
The state layer renders before the slot content, so media paints over it. On web, hovering the "Filled (default)" Card darkens only the area below the cover. The image pixels do not change at all. Focus and dragged layers have the same problem. Material draws state layers over the whole component, so render this Animated.View after the content (it already has pointerEvents="none").
(via Claude, verify if it makes sense)
| const content = ( | ||
| <View style={[styles.innerContainer, contentStyle]}> | ||
| {React.Children.map(children, (child, index) => | ||
| React.isValidElement(child) | ||
| ? React.cloneElement(child as React.ReactElement<any>, { | ||
| index, | ||
| total, | ||
| siblings, | ||
| }) | ||
| : child | ||
| )} | ||
| <View | ||
| style={[styles.content, contentStyle]} | ||
| testID={hasPassedTouchHandler ? undefined : testID} |
There was a problem hiding this comment.
A disabled Card dims only its container or outline. The title, subtitle and content keep full contrast, so the disabled outlined Card on the example screen looks the same as an enabled one with a fainter border. Compose CardDefaults.cardColors, elevatedCardColors and outlinedCardColors set disabledContentColor to the content color at 38%. Apply the disabled opacity to this content wrapper when isDisabled is true, or at least to the header that Card renders from title and subtitle.
(via Claude, verify if it makes sense)
| jest.mock('react', () => jest.requireActual('../../../../node_modules/react')); | ||
| jest.mock('react-native-reanimated', () => | ||
| jest.requireActual('../../../../node_modules/react-native-reanimated') | ||
| ); | ||
| jest.mock('react-native-paper', () => | ||
| jest.requireActual('../../../../src/index') | ||
| ); |
There was a problem hiding this comment.
This suite fails to run in a clean checkout:
Cannot find module 'react-native-paper' from 'example/src/Examples/__tests__/CardRenderCountExample.test.tsx'
yarn test ends with 1 failed suite (57 suites, not the 58 in the PR description). jest.mock resolves the module name before it uses the factory, and nothing maps react-native-paper at the repo root. The manual mocks of react and react-native-reanimated by relative path also work around the example workspace resolution. Card.test.tsx already has "does not rerender stable memoized slot content for transient feedback", which checks the same behavior through the public API. Remove this file, or move any extra case into Card.test.tsx. (via Claude, verify if it makes sense)
|
|
||
| Paper 6 warns in development if whole-Card interaction handlers and a populated `actions` slot are combined. A neutral Card remains a grouping container unless you provide accessibility semantics explicitly. The `dragged` prop controls the Material dragged presentation only; drag gestures and lifecycle remain application responsibilities. | ||
|
|
||
| Card refs and test IDs now target documented nodes: `ref` targets the outer shell, `touchableRef` targets the actionable interaction node, and `testID` targets the interaction node for actionable Cards or the slot-content node for neutral Cards. `${testID}-container` and `${testID}-visual` identify the outer shell and clipped visual region. |
There was a problem hiding this comment.
efe5d34 removed the derived IDs, so the last sentence describes IDs that Card no longer renders.
| Card refs and test IDs now target documented nodes: `ref` targets the outer shell, `touchableRef` targets the actionable interaction node, and `testID` targets the interaction node for actionable Cards or the slot-content node for neutral Cards. `${testID}-container` and `${testID}-visual` identify the outer shell and clipped visual region. | |
| Card refs and test IDs now target documented nodes: `ref` targets the outer shell, `touchableRef` targets the actionable interaction node, and `testID` targets the interaction node for actionable Cards or the slot-content node for neutral Cards. |
(via Claude, verify if it makes sense)
| title={title} | ||
| content={ | ||
| <Card.Content> | ||
| <Text variant="titleLarge">{title}</Text> |
There was a problem hiding this comment.
The Card now gets title={title}, so this line renders the title a second time inside the content.
| <Text variant="titleLarge">{title}</Text> |
(via Claude, verify if it makes sense)
| // styles are guarded with a snapshot of the rendered tree instead. | ||
| const expectVisualSnapshot = () => { | ||
| expect(screen.toJSON()).toMatchSnapshot(); | ||
| }; | ||
|
|
||
| afterEach(() => { |
There was a problem hiding this comment.
Most visual tests assert through this helper, which produces an 11,612-line snapshot file. The test rules ask to avoid assertions on exact internal style values, and a snapshot of the whole tree is that, at a size nobody can review. It also needed the jest/expect-expect change in eslint.config.mjs. The token matrix already has direct tests in Card.tokens.test.ts. Keep a few small snapshots for the variants and assert behavior (roles, disabled state, callbacks) for the rest. (via Claude, verify if it makes sense)
| <View | ||
| style={[ | ||
| styles.verification, | ||
| { borderColor: theme.colors.outlineVariant }, | ||
| ]} | ||
| > | ||
| <Text variant="titleMedium">Platform verification</Text> | ||
| <Text variant="bodySmall"> | ||
| Android · iOS · web: compare layout, both themes, surface roles, | ||
| outlines, clipping, and elevation. | ||
| </Text> | ||
| <Text variant="bodySmall"> | ||
| Web: use Tab and hover on actionable Cards to inspect focus and state | ||
| layers. | ||
| </Text> | ||
| <Text variant="bodySmall"> | ||
| Native: touch actionable Cards to inspect bounded ripple, clipping, | ||
| and elevation. Current platform: {Platform.OS}. | ||
| </Text> | ||
| </View> | ||
|
|
||
| <View style={styles.section}> | ||
| <CardRenderCountExample /> | ||
| </View> |
There was a problem hiding this comment.
The example app ships to the app stores now (#5118). The "Platform verification" block gives review instructions ("use Tab and hover ... to inspect focus and state layers"), and CardRenderCountExample is a render-count benchmark. Both are QA tools, not Card demos. Move them out of the gallery. The previous example also had a variant switcher and covered more layouts, such as a cover with only actions and custom button styles. (via Claude, verify if it makes sense)
| const disabledState = useSharedValue(isDisabled); | ||
| const draggedState = useSharedValue(dragged); | ||
| const hovered = useSharedValue(false); | ||
| const focused = useSharedValue(false); | ||
| const pressed = useSharedValue(false); |
There was a problem hiding this comment.
Hover, focus, press, drag and disabled are changes between fixed states. Nothing here follows a gesture or a scroll position. So these fit Reanimated CSS transitions with React state, as in Checkbox and Badge on main, better than shared values. This replaces the earlier suggestion to call withTiming inside the worklets.
Keep hovered, focused and pressed in useState, pick the visuals during render, and pass plain styles:
const [pressed, setPressed] = React.useState(false);
const current = isDisabled
? disabledVisuals
: dragged
? draggedVisuals
: pressed
? pressedVisuals
: focused
? focusedVisuals
: hovered
? hoveredVisuals
: enabledVisuals;
<Animated.View
style={[
styles.stateLayer,
{
opacity: current.stateLayerOpacity,
transitionProperty: 'opacity',
transitionDuration,
transitionTimingFunction,
},
]}
/>The transitions then start when React commits the new style, disabled and dragged need no effect to copy them, and the shared values and useDerivedValue go away. The cost is one Card render per state change. (via Claude, verify if it makes sense)
| * A Reanimated shared value can drive elevation without a React render. | ||
| */ | ||
| elevation?: Elevation; | ||
| elevation?: Elevation | SharedValue<Elevation>; |
There was a problem hiding this comment.
With React state in Card, Card can pass a plain elevation here. Surface on main already runs background, elevation and shadow changes through CSS transitions, so a state change in Card animates with no extra code. An elevation change on hover or press is a state transition, not gesture or scroll driven motion, so it does not need a shared value. Please drop SharedValue<Elevation> and AnimatedElevationSurface. That path also skips the CSS transitions, as the comment on Card.tsx notes. (via Claude, verify if it makes sense)
| borderRadiusStyles, | ||
| }); | ||
| const CardCover = ({ style, theme: _theme, ...rest }: Props) => { | ||
| const usesAspectRatio = StyleSheet.flatten(style)?.aspectRatio !== undefined; |
There was a problem hiding this comment.
This flattens the whole style prop on every render to read one value, and the layout now depends on what the style happens to contain. A dedicated prop states the intent and needs no StyleSheet.flatten:
const CardCover = ({ aspectRatio, style, theme: _theme, ...rest }: Props) => (
<Image
{...rest}
style={[
styles.image,
aspectRatio === undefined ? styles.defaultHeight : { aspectRatio },
style,
]}
accessibilityIgnoresInvertColors
/>
);The prop docs, the usage example and the migration guide would then use aspectRatio={16 / 9}. (via Claude, verify if it makes sense)
Motivation
Cardwas still on the Paper 5 model: amodeprop, arbitrary children composition, and no first-class support for Material 3 expressive states (hover, focus, press, drag) or emphasis tokens. This made it hard to build Cards that match the Material 3 spec — variant-driven container/outline/elevation colors, state layers, focus indicators, and motion — without consumers hand-rolling that behavior on top ofView/Surface.This PR rewrites
Cardaround the Material 3 contract:mode+ arbitrary children with an explicitvariant(filled|elevated|outlined) and deterministic slots (media,header/title+subtitle+leading+trailing,content,actions).actionsslot.resolveCardVisualstoken contract, including MD3-correct color tokens (surfaceContainerHighestfor filled,outlineVariantfor outlined borders) and real ripple/state-layer feedback viaTouchableRippleinstead of a barePressable.Related issue
Addresses #4929.
Screenshots / Videos
See the new "Expressive Card gallery" example (
example/src/Examples/CardExample.tsx) for variants, states, shapes, and light/dark/platform verification, andCardRenderCountExample.tsxfor the render-count regression check.Test plan
yarn jest— full suite passes (58 suites / 737 tests), including the rewrittenCard,Card.tokens, andSurfaceunit tests covering variants, states (hover/press/focus/drag/disabled), shapes, accessibility, and motion.yarn typecheck— passes, including the new@ts-expect-errorcases asserting the removedmode/children API is rejected at compile time.yarn lint— passes.CardExamplegallery in the example app (variants, custom headers, responsive cover media, disabled/dragged states, light/dark theme, platform-specific pressed/long-press messaging).