Conversation
| ]} | ||
| > | ||
| {icon ? ( | ||
| <View style={styles.rippleContent} /> |
There was a problem hiding this comment.
Try keeping the label inside TouchableRipple, or deriving aria-label from it - as a pointerEvents="none" sibling it never reaches the button, so getByRole('button', { name: 'Example Chip' }) resolves on main and returns null here. Worth a test to pin it.
| selectedIconColor: 'onSecondaryContainer', | ||
| trailingIconColor: 'onSurfaceVariant', | ||
| selectedTrailingIconColor: 'onSecondaryContainer', | ||
| outlineColor: 'outline', |
There was a problem hiding this comment.
Was outline intentional here? main uses outlineVariant (helpers.tsx:48), and all four live chip namespaces resolve to outline-variant — the only token giving outline is md.comp.input-chip.outline.color, which is deprecated.
| theme={theme} | ||
| > | ||
| <Icon | ||
| source={hasTrailingIcon ? trailingIcon! : (closeIcon ?? 'close')} |
There was a problem hiding this comment.
Try dropping the ! - it typechecks without one, and @typescript-eslint/no-non-null-assertion is error-level on main (eslint.config.mjs:150).
| aria-label={ | ||
| hasTrailingIcon | ||
| ? trailingIconAccessibilityLabel | ||
| : closeIconAccessibilityLabel | ||
| } |
There was a problem hiding this comment.
Worth giving trailingIconAccessibilityLabel a default, or requiring it alongside onTrailingIconPress - <Chip trailingIcon="menu-down" onTrailingIconPress={…}> currently renders a role="button" with no accessible name, where the close button defaults to 'Close'.
| const styles = StyleSheet.create({ | ||
| container: { | ||
| borderWidth: StyleSheet.hairlineWidth, | ||
| height: ChipTokens.containerHeight, |
There was a problem hiding this comment.
Try minHeight here - with overflow: 'hidden' on the content (line 438) a fixed height clips the label at larger system font sizes. main reached the same 32dp via lineHeight: 20 plus marginVertical: 6, so it scaled.
- height: ChipTokens.containerHeight,
+ minHeight: ChipTokens.containerHeight,| */ | ||
| const Chip = ({ | ||
| mode = 'flat', | ||
| mode = 'outlined', |
There was a problem hiding this comment.
Worth adding a Chip section to migration.md - compact and showSelectedOverlay are gone and this default flip changes every existing <Chip>. The TextInput entry is a template, and the generated docs are already done.
| disabledColor: 'onSurface', | ||
| } as const satisfies Record<string, ColorRole>; | ||
|
|
||
| export const ChipTokens = { ...sizes, ...colors }; |
|
#5078 landed and reworked |
| color: textColor, | ||
| ...theme.fonts.labelLarge, | ||
| const leftPadding = hasAvatar | ||
| ? ChipTokens.avatarLeadingPadding |
There was a problem hiding this comment.
This reserves the whole 32dp touch target as padding, so the label ends up ~15dp from the trailing icon where the specs page says 8dp between elements. Should this count the icon rather than the touch target?
ed1803a to
b13784e
Compare
b13784e to
b93dc8f
Compare
|
Found potential problems with the pull request:
|
- rework Chip around a dedicated tokens.ts (colors/sizes) and utils.ts (was helpers.tsx), replacing legacy RN-core Animated with Surface's Reanimated-driven elevation - add trailingIcon prop independent of onClose, plus M3 assist/filter/ input/suggestion example variants - wire up hover-driven elevation, focus rings (chip body + trailing icon), and focus-outline darkening - fix disabled+selected chips losing their filled background, and trailing-icon-to-label spacing using the touch target instead of the icon's own size
290eb3a to
e74866d
Compare
Motivation
Refactors
Chiptoward the v6/MD3 API and implementation direction.Chip-specific sizing, color, and typescale values into a dedicatedtokens.tsfile, following the pattern used by other recently modernized components (e.g.Badge).Animated.timingdriven scale/elevation on press) in favor of a static elevation derived from theelevatedanddisabledprops.getChipColorsinutils.ts, renamed fromhelpers.tsx) to better align with MD3 state and color roles, including a dedicated ripple color and an avatar selected-overlay color.hitSlop, and moves the close icon intoTouchableRipplefor consistent ripple/state-layer feedback.compactandshowSelectedOverlayprops, which are no longer part of the MD3-aligned API (the selected overlay is now handled automatically for avatars).modefromflattooutlined.The example app has been updated to match the new API (e.g. dropping now-removed
showSelectedOverlayusage) and to showcase MD3-oriented Chip states.Related issue
Closes #4931
Related PRs
Based on the #5002
Wait for #5084 to be merged to take advantage of the focus indicators in TouchableRipple
Test plan