Repository navigation
feat: add ConnectedButtonGroup component - #5074
oleksandrzavarzin-callstack wants to merge 18 commits into
Conversation
…group Introduces ConnectedButtonGroup, the MD3 successor to SegmentedButtons. Selected buttons morph to a fully-rounded shape, the connected inner corners expand on press, and the group supports single- and multi-select across the extra-small to extra-large size scale. Component-specific tokens are extracted into tokens.ts; size, shape and color resolution live in utils.ts. SegmentedButtons is marked as deprecated in favour of the new component.
…ty overlay Pre-blending the disabled container color with color().alpha() breaks on Android dynamic themes where onSurface is a PlatformColor object rather than a string — the fallback rendered a fully opaque near-black container. Apply the MD3 12% disabled opacity as a style on an absolute-fill overlay instead, which works with any ColorValue. Found during on-device Android verification.
- tokens: pressed inner corner now morphs sharper than rest (M3 ConnectedButtonGroup*Tokens: Inner=Small, PressedInner=ExtraSmall), and large/extra-large rest inner corners corrected to large(16)/largeIncreased(20). - press morph: selected buttons also morph on press (pressed precedence over checked, matching Compose ToggleButton); pressOut returns to the rest radius. - accessibility: buttons now use the radio (single-select) / checkbox (multi-select) role instead of button+aria-checked, valid on web and better announced natively. - reduce-motion: shape morph honours the reduce-motion setting (ReduceMotion), matching Switch/FAB. - docs: register ConnectedButtonGroup in component-docs.config so the deprecation link and generated page resolve. - tests: cover shape tokens and role semantics (22 total).
- resolve corner shapes through the shared ShapeToken/resolveCornerRadius from theme/utils/shape instead of a local re-implementation. - drop the per-button rippleColor prop and runtime ripple color derivation; TouchableRipple's stateLayerPressed default applies, consistent with the removal of customRippleColor across components. - expose background and hitSlop passthroughs per button, mirroring SegmentedButtons, and remove the ineffective automatic touch-target expansion. - drive the corner morph with the theme's fast spatial spring (toRawSpring), matching Switch and FAB, instead of timing curves. - add radiogroup semantics to the single-select row and collision-proof list keys. - honour intrinsic button widths in content-sized parents (flexBasis auto). - export ConnectedButtonGroupSize, register a docs screenshot, keep the docs config alphabetical, and extend tests with render snapshots (23).
The docs build requires a committed page under 5.x/docs/components for every entry in component-docs.config; add the ConnectedButtonGroup page (usage, props, theme colors) mirroring SegmentedButtons, regenerate the 6.x content, and point the SegmentedButtons deprecation notice at the correct relative path so the link resolves.
oleksandrzavarzin-callstack
changed the title
|
@MikitasK fixed your comments. |
|
@JKobrynski could you please review this Pull Request? |
|
Review by Claude Web
|
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
docs/public/screenshots/connected-button-group.pngis out of date. It shows square inner corners on "Driving" and "Left", but the component now renders thesmallinner radius there. Please capture it again from the current example. (via Claude, verify if it makes sense)
| const { start: animateStart, end: animateEnd } = morphingCorners[position]; | ||
|
|
||
| // A single spring owns the morphing radius, derived from the current props | ||
| // rather than written by the press handlers and the selection separately — | ||
| // those used to fire competing springs on one shared value, so a tap ran | ||
| // three transitions (pressed, old rest, new rest) instead of one. | ||
| const cornerRadius = useDerivedValue( | ||
| () => withSpring(pressed.value ? pressedRadius : restRadius, springConfig), | ||
| [restRadius, pressedRadius, springConfig] | ||
| ); | ||
|
|
||
| const animatedShapeStyle = useAnimatedStyle(() => { | ||
| // Read the plain number. A `withSpring` result is a stateful animation | ||
| // object and must never be handed to more than one style prop: four | ||
| // corners sharing one object step the same state machine four times a | ||
| // frame and desync. | ||
| const morph = cornerRadius.value; | ||
| const startRadius = animateStart ? morph : outerRadius; | ||
| const endRadius = animateEnd ? morph : outerRadius; | ||
| return { | ||
| borderTopStartRadius: startRadius, | ||
| borderBottomStartRadius: startRadius, | ||
| borderTopEndRadius: endRadius, | ||
| borderBottomEndRadius: endRadius, | ||
| }; | ||
| }, [animateStart, animateEnd, outerRadius]); |
There was a problem hiding this comment.
On web the corners never change after the first render. Reanimated's web path writes the animated style straight to element.style and skips react-native-web's preprocessing, which is the step that maps borderTopStartRadius to a real CSS property. Browsers ignore element.style.borderTopStartRadius, so both the selection morph and the press morph are dropped.
This is also a transition between states (rest, selected, pressed), not gesture or scroll driven motion, so per the project rule it should use Reanimated CSS transitions instead of a shared value with useDerivedValue and withSpring, as in Checkbox.tsx. The CSS logical names map to real CSS properties, so they also work on web:
const [pressed, setPressed] = React.useState(false);
const morphRadius = pressed ? pressedRadius : restRadius;
const shapeStyle: CSSStyle<ViewStyle> = {
borderStartStartRadius: animateStart ? morphRadius : outerRadius,
borderEndStartRadius: animateStart ? morphRadius : outerRadius,
borderStartEndRadius: animateEnd ? morphRadius : outerRadius,
borderEndEndRadius: animateEnd ? morphRadius : outerRadius,
transitionProperty: [
'borderStartStartRadius',
'borderEndStartRadius',
'borderStartEndRadius',
'borderEndEndRadius',
],
transitionDuration: reduceMotion ? 0 : theme.motion.duration.short3,
transitionTimingFunction: cubicBezier(...theme.motion.easing.standard),
};With these property names the radii animate on iOS, Android and web. Keep borderTopStartRadius and friends out of transitionProperty: on web they become border-top-start-radius, which is not a CSS property, so the transition does not run. This also removes the derived value and the comments about competing springs and shared animation objects. Pick the duration and easing tokens that match the spec. (via Claude, verify if it makes sense)
| const checkScale = useSharedValue(showCheck ? 1 : 0); | ||
|
|
||
| const reduceMotion = useReduceMotion(); | ||
| const springConfig = React.useMemo( | ||
| () => ({ | ||
| ...toRawSpring(theme.motion.spring.fast.spatial), | ||
| reduceMotion: reduceMotion ? ReduceMotion.Always : ReduceMotion.Never, | ||
| }), | ||
| [theme.motion.spring.fast.spatial, reduceMotion] | ||
| ); | ||
|
|
||
| React.useEffect(() => { | ||
| // Springs to the value it already holds on mount, which is a no-op. | ||
| checkScale.value = withSpring(showCheck ? 1 : 0, springConfig); | ||
| }, [showCheck, checkScale, springConfig]); |
There was a problem hiding this comment.
The check and leading icon scale is a transition between selection states, so per the project rule it should use Reanimated CSS transitions instead of a shared value that an effect drives with withSpring. The current approach also depends on fragile timing, as the comment on lines 146 to 152 says: it relies on useAnimatedStyle caching the first render and on the effect running after paint.
One option: keep the check and the leading icon mounted in one leading slot and transition transform (scale) and opacity on each, as Checkbox does with its mask. For the icon-only case, where both show at once, the check slot can transition its width, as Checkbox does. That removes checkScale, the effect and both animated styles. (via Claude, verify if it makes sense)
| aria-label={ariaLabel} | ||
| aria-disabled={disabled} | ||
| aria-checked={checked} | ||
| role={multiSelect ? 'checkbox' : 'radio'} |
There was a problem hiding this comment.
On web, Space does not activate these buttons. react-native-web's press responder handles Space only for role="button", so a focused radio or checkbox ignores it. Enter works. SegmentedButtons uses role="button", so Space works there today, and people who migrate lose it. Checkbox has the same gap, so a fix in TouchableRipple may fit better than one here.
The single-select group also does not follow the radio group keyboard pattern: every radio is its own Tab stop and the arrow keys do nothing. Please handle Space, and for the single-select group add arrow key navigation with one Tab stop on the checked radio. (via Claude, verify if it makes sense)
|
|
||
| return ( | ||
| <View | ||
| role={multiSelect ? undefined : 'radiogroup'} |
There was a problem hiding this comment.
The group has no accessible name, and Props gives no way to pass one, so screen readers announce an unnamed radio group. The multi-select group gets no role at all, so its checkboxes are not grouped. Please accept aria-label and aria-labelledby on the group, and use role="group" for multi-select:
| role={multiSelect ? undefined : 'radiogroup'} | |
| role={multiSelect ? 'group' : 'radiogroup'} |
(via Claude, verify if it makes sense)
| /** | ||
| * Value of the currently selected button, or an array of the selected | ||
| * values when `multiSelect` is enabled. | ||
| */ | ||
| value: T | T[]; | ||
| /** | ||
| * Allow more than one button to be selected at a time. | ||
| */ | ||
| multiSelect?: boolean; | ||
| /** | ||
| * Function to execute on selection change. | ||
| */ | ||
| onValueChange: ((value: T) => void) | ((value: T[]) => void); |
There was a problem hiding this comment.
Declaring value, multiSelect and onValueChange here and again in ConditionalValue makes the generated docs harder to read. The docs parser now prints value as T | T[] | (T & T[]) | (T[] & T), and onValueChange as an intersection of both callback unions. With only ConditionalValue, as in SegmentedButtons, the same parser lists all three props and prints T | T[] and ((value: T[]) => void) | ((value: T) => void).
Please remove these lines. The parser takes the description from the first branch of ConditionalValue, so also give value the same JSDoc in both branches, for example "Value of the currently selected button, or an array of the selected values when multiSelect is enabled."
| /** | |
| * Value of the currently selected button, or an array of the selected | |
| * values when `multiSelect` is enabled. | |
| */ | |
| value: T | T[]; | |
| /** | |
| * Allow more than one button to be selected at a time. | |
| */ | |
| multiSelect?: boolean; | |
| /** | |
| * Function to execute on selection change. | |
| */ | |
| onValueChange: ((value: T) => void) | ((value: T[]) => void); |
(via Claude, verify if it makes sense)
| /** | ||
| * Size of the buttons, following the Material Design 3 button-group scale. | ||
| */ |
There was a problem hiding this comment.
The props table shows the type of size as ConnectedButtonGroupSize, which does not tell readers the allowed values. Please list them:
| /** | |
| * Size of the buttons, following the Material Design 3 button-group scale. | |
| */ | |
| /** | |
| * Size of the buttons, following the Material Design 3 button-group scale: | |
| * `'extra-small'`, `'small'`, `'medium'`, `'large'` or `'extra-large'`. | |
| */ |
(via Claude, verify if it makes sense)
| * The overlap with `ConnectedButtonConfig` is deliberate: the docs generator | ||
| * only expands the `buttons` API table when the config is an inline object | ||
| * literal in `ConnectedButtonGroup.tsx`. |
There was a problem hiding this comment.
The stated reason does not apply here. ConnectedButtonConfig is a named type alias, not an inline object literal, and the docs parser prints buttons as ConnectedButtonConfig<T>[]. So the three copies of the button fields (this type, ConnectedButtonConfig, and the bullet list in the buttons JSDoc) give no docs benefit, and they already drift: the bullet list leaves out background, hitSlop and labelMaxFontSizeMultiplier. Please move ConnectedButtonConfig into this file and derive the button props from it:
export type ConnectedButtonProps = Omit<ConnectedButtonConfig, 'value'> & {
checked: boolean;
multiSelect?: boolean;
position: ConnectedButtonPosition;
size: ConnectedButtonGroupSize;
theme?: ThemeProp;
};(via Claude, verify if it makes sense)
| describe('connected button shape tokens', () => { | ||
| it('presses the inner corner sharper than its resting radius (M3)', () => { | ||
| // small: inner = small (8dp), pressed inner = extraSmall (4dp) | ||
| const { innerRadius, pressedRadius, outerRadius } = | ||
| getConnectedButtonSizeStyle({ size: 'small', theme }); | ||
| expect(pressedRadius).toBeLessThan(innerRadius); | ||
| expect(innerRadius).toBe(theme.shapes.corner.small); | ||
| expect(pressedRadius).toBe(theme.shapes.corner.extraSmall); | ||
| // outer edge stays fully rounded | ||
| expect(outerRadius).toBeGreaterThan(innerRadius); | ||
| }); | ||
|
|
||
| it('resolves the pill corner to half the height, not the full-corner sentinel', () => { | ||
| const sizes: ConnectedButtonGroupSize[] = [ | ||
| 'extra-small', | ||
| 'small', | ||
| 'medium', | ||
| 'large', | ||
| 'extra-large', | ||
| ]; | ||
| expect(sizes).toHaveLength(Object.keys(connectedButtonSizeTokens).length); | ||
|
|
||
| sizes.forEach((size) => { | ||
| const { outerRadius, containerHeight } = getConnectedButtonSizeStyle({ | ||
| size, | ||
| theme, | ||
| }); | ||
|
|
||
| expect(connectedButtonSizeTokens[size].outerShape).toBe('full'); | ||
| expect(outerRadius).toBe(containerHeight / 2); | ||
| expect(outerRadius).toBeLessThan(cornerFull); | ||
| }); | ||
| }); | ||
|
|
||
| it('uses the spec inner corners for large and extra-large', () => { | ||
| expect(connectedButtonSizeTokens.large.innerShape).toBe('large'); | ||
| expect(connectedButtonSizeTokens['extra-large'].innerShape).toBe( | ||
| 'largeIncreased' | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getConnectedButtonPosition', () => { | ||
| it('classifies a lone button as single', () => { | ||
| expect(getConnectedButtonPosition(0, 1)).toBe('single'); | ||
| }); | ||
|
|
||
| it('classifies first, middle and last positions', () => { | ||
| expect(getConnectedButtonPosition(0, 3)).toBe('first'); | ||
| expect(getConnectedButtonPosition(1, 3)).toBe('middle'); | ||
| expect(getConnectedButtonPosition(2, 3)).toBe('last'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getConnectedButtonColors', () => { | ||
| it('uses MD3 selection color roles', () => { | ||
| const selected = getConnectedButtonColors({ theme, selected: true }); | ||
| expect(selected.containerColor).toBe(theme.colors.secondaryContainer); | ||
| expect(selected.contentColor).toBe(theme.colors.onSecondaryContainer); | ||
|
|
||
| const unselected = getConnectedButtonColors({ theme, selected: false }); | ||
| expect(unselected.containerColor).toBe(theme.colors.surfaceContainer); | ||
| expect(unselected.contentColor).toBe(theme.colors.onSurfaceVariant); | ||
| }); | ||
|
|
||
| it('dims content and keeps onSurface when disabled', () => { | ||
| const disabled = getConnectedButtonColors({ | ||
| theme, | ||
| selected: false, | ||
| disabled: true, | ||
| }); | ||
| expect(disabled.contentColor).toBe(theme.colors.onSurface); | ||
| expect(disabled.contentOpacity).toBe(0.38); | ||
| }); | ||
|
|
||
| it('honours custom checked and unchecked colors', () => { | ||
| expect( | ||
| getConnectedButtonColors({ | ||
| theme, | ||
| selected: true, | ||
| checkedColor: 'red', | ||
| }).contentColor | ||
| ).toBe('red'); | ||
| expect( | ||
| getConnectedButtonColors({ | ||
| theme, | ||
| selected: false, | ||
| uncheckedColor: 'blue', | ||
| }).contentColor | ||
| ).toBe('blue'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Many tests here check internal details instead of behavior. Lines 50 to 60, 109 to 118, 196 to 214 and 252 to 291 assert exact height, backgroundColor, marginEnd and corner radius values on internal wrappers. The tests from line 293 call getConnectedButtonPosition, getConnectedButtonColors and the token table directly, and mostly restate the source. The project test rules ask for public behavior and accessibility queries. Selection and disabled state are never asserted through accessibility. These queries work with the current component:
expect(screen.getByRole('radio', { name: 'Walking' })).toBeChecked();
expect(screen.getByRole('radio', { name: 'Transit' })).not.toBeChecked();
expect(screen.getByRole('radio', { name: 'Transit' })).toBeDisabled();Please replace the style and helper tests with tests like these, and use getByRole instead of test IDs for presses. (via Claude, verify if it makes sense)
|
Review by Claude Web
|
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Review by Claude
See the inline comments. (via Claude, verify if it makes sense)
| // whenever the selection check mounts or unmounts, which reads as the | ||
| // buttons snapping to a new width on every tap. | ||
| flex: 1, | ||
| overflow: 'hidden', |
There was a problem hiding this comment.
On web this clips the keyboard focus ring. The focused element is the inner TouchableRipple, and its outline is drawn outside its box, so this container cuts it down to two thin lines at the top and bottom, with no ring on the rounded ends. SegmentedButtons has no overflow: 'hidden' on its wrapper and draws a full ring.
Please draw the focus ring where the container does not clip it, for example on the container itself, so keyboard users can see which button has focus. (via Claude, verify if it makes sense)
| * Sets additional distance outside of the button in which a press can be | ||
| * detected. | ||
| */ | ||
| hitSlop?: TouchableRippleProps['hitSlop']; |
There was a problem hiding this comment.
hitSlop has no effect. React Native never extends the touch area past the parent view, and the parent of the TouchableRipple is the container view, which has the same size as the button. With hitSlop: { top: 12, bottom: 12 } on an extra-small button, a tap 6 dp below the button does nothing on iOS and Android, while a tap inside it selects it. This matters most for the 32 dp extra-small size, which is below the 48 dp touch target. Please make the pressable the outermost view of the button, or remove the prop so it does not promise a larger touch area. (via Claude, verify if it makes sense)
Motivation
Continues #5028. Addresses @alpharius-ck's review comments, and fixes two corner-animation bugs found while verifying those fixes on device: a button that could get stuck in the wrong shape, and a square flash on the middle button of a group.
Related issue
Review comments on #5028.
What changed and why
Review comments:
SegmentedButtonsand added a note onmigrating
density→size.SegmentedButtonItem.connectedButtonPositionsand amorphingCornerslookup table.getTestIDout of the component.types.ts.Animation bugs:
three spring transitions instead of one. Fixed by deriving the radius from
current props in a single
useDerivedValue.swallowed
onPressOutcould strand the button in its pressed shape. Fixedby also clearing the pressed state on press.
cornerFullis a sentinel value of9999, not a real radius — animating toit caused a large spring overshoot that flashed the corner square. Now
resolved to
containerHeight / 2instead.The press-in morph itself isn't covered by a test: the reanimated jest mock
doesn't track shared-value mutations, so it's verified manually instead,
matching how
Switch.tsxalready handles this.Test plan
yarn typecheck,yarn lint— clean.yarn test ConnectedButtonGroup— 28 tests (up from 23).connected corner on both selected and unselected buttons; repeated taps on
the selected button never strand the shape; the middle button no longer
flashes square; deselecting a labelled button animates the icon back in.
Screen.Recording.2026-08-25.at.12.58.09.mov