refactor: replace React.Children usage to more composition-friendly alternatives (#4989)#5018
Open
matkoson wants to merge 12 commits into
Open
refactor: replace React.Children usage to more composition-friendly alternatives (#4989)#5018matkoson wants to merge 12 commits into
matkoson wants to merge 12 commits into
Conversation
…th context List.Accordion used React.Children.map + cloneElement to inject paddingLeft and theme into expanded children, which makes composition harder (children had to be direct, prop-mergeable elements). Replace it with ListAccordionContext: the accordion exposes whether descendants should indent (leftIndent), and List.Item consumes it, applying the indent to its own container so the ripple/background stays full-width. Behavior preserved for List.Item children; theme now flows via context. Adds characterization tests. Refs callstack#4989.
matkoson
changed the title
Dialog cloned its children with React.Children + cloneElement to inject the theme, and DialogActions inspected the child count/index to space the first and last action. Both couple the components to the exact shape of their children. Remove the cloning: Dialog.Title/Content/Actions/Icon resolve the theme themselves via useInternalTheme, and DialogActions spaces its actions with a container `gap` instead of per-child injection. BREAKING CHANGE: Dialog no longer clones its children to forward an injected theme. The subcomponents read the theme from context (PaperProvider) or an explicit `theme` prop, so pass `theme` directly to a Dialog subcomponent if you previously relied on Dialog forwarding it. Wrapping or conditionally rendering Dialog.Actions children no longer affects their spacing. Refs callstack#4989
Card used React.Children.count/map + cloneElement to inspect sibling position and inject props (the theme, and top padding for the first child) into Card.Content/Cover/Title/Actions. That makes composition fragile: children had to be direct, recognised elements. Render the children directly instead and let each subcomponent own its own theme resolution and padding. BREAKING CHANGE: Card no longer clones its children to inject a theme or sibling-derived padding. Card.Content/Cover/Title/Actions read the theme from context or an explicit `theme` prop and apply their own spacing, so wrapping or reordering them keeps working; pass `theme` directly if you previously relied on Card forwarding it. Refs callstack#4989
ToggleButton.Row used React.Children.count/map + cloneElement to inject a first/middle/last border radius into each button by position, which only works when the buttons are direct children. Replace it with ToggleButtonRowContext: the row flags its descendants as segmented, each ToggleButton reads the flag and renders the flat segment shape itself, and the row clips a single rounded container with hairline dividers between segments. BREAKING CHANGE: ToggleButton.Row no longer clones its children to inject per-segment border radii. Segment styling now comes from ToggleButtonRowContext, so the buttons may be wrapped or conditionally rendered and still pick up the segmented appearance. Refs callstack#4989
Use the MD3 segmented-button shape token (corner.largeIncreased) for the row container and secondaryContainer for the selected segment, matching the existing SegmentedButtons component. Full per-segment alignment (first/middle/last radius as in SegmentedButtons) is intentionally NOT done: it would require positional child inspection, the React.Children anti-pattern this refactor removes. The container-clip + divider approach keeps the segmented look composition-friendly. Refs callstack#4989
Replace the `cloneElement` prop injection and `displayName`-based filtering in Appbar with an `AppbarContext` that exposes the shared `isDark` and `mode` values. `Appbar.Action`, `Appbar.BackAction` and `Appbar.Content` now read those values from context and derive their own foreground color, instead of having `color`/`mode`/`theme` spliced into them. - `small` and `center-aligned` render their children directly in author order; the title's `flex: 1` keeps trailing actions right-aligned and the center-aligned title centers itself. - `medium` and `large` keep the two-row layout (controls row above a full-width title) by partitioning children for placement only — no props are injected. - Drops the `React.Children.forEach` count heuristic that conditionally centered the title in `center-aligned`. BREAKING CHANGE: Appbar children are now rendered in the order they are written rather than being reordered to put the back action first. Place `Appbar.BackAction` before `Appbar.Content` and trailing `Appbar.Action`s after it, as shown in the docs. Children no longer receive injected `color`/`mode`/`theme` props; wrapping `Appbar.Content`/`Appbar.Action` in another element keeps working because the values come from context.
The composition refactor's job is to remove React.Children/cloneElement
without changing what users see. A separate commit on this branch
("align segmented row to MD3 spec") went further: it changed
ToggleButton.Row's corner radius (extraSmall -> largeIncreased) and the
selected segment's fill (own color -> secondaryContainer), converging it
toward the MD3 SegmentedButtons component.
That redesign was scope creep. ToggleButton.Row was never styled to the
MD3 SegmentedButtons spec upstream (it used surfaceContainerHighest /
extraSmall corners, distinct from SegmentedButtons' secondaryContainer /
largeIncreased) -- it's a separate, legacy component identity, not an
under-spec implementation of SegmentedButtons. The acceptance criterion
that triggered the MD3 pass ("double-check any affected component still
follows the specs") is a non-regression check, not a redesign mandate.
Revert ToggleButton.Row's corner back to theme.shapes.corner.extraSmall
and ToggleButton's selected fill back to the unconditional
getToggleButtonColor(...) (no row-specific override), matching the
original upstream component exactly. The composition-safe technique from
the prior commit (ToggleButtonRowContext, container-clip + gap divider)
is untouched -- this only reverts the two MD3-redesign value changes on
top of it, restoring a visually transparent composition refactor.
Verified: tsc clean, eslint clean, ToggleButton suite 8/9 (the 1 failure
is the pre-existing baseline animation test, unchanged). Pixel diff
against the prior B+ screenshot confirms the change is real and scoped
to exactly the Row control (4156px, corner + selected-fill only) with
zero diff elsewhere on screen.
matkoson
force-pushed
the
refactor/react-children
branch
from
9a9da84 to
b3d0b92
Compare
There was a problem hiding this comment.
Pull request overview
Refactors several UI components to avoid React.Children inspection and cloneElement, improving composition (wrapped children, custom components) by moving styling/spacing responsibility into contexts and container-owned layout.
Changes:
- Reworks
ToggleButton.Rowto use context-driven segmented styling (no positional cloning) and adds optional theming. - Updates
List.Accordionindentation behavior via context so wrappedList.Itemchildren indent correctly without prop injection. - Refactors
Dialog,Dialog.Actions, andCardsections to remove child cloning, shifting spacing/shape behavior to containers and updating tests/examples accordingly.
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/ToggleButton/ToggleButtonRowContext.tsx | Adds context to signal segmented-row rendering to descendant toggle buttons. |
| src/components/ToggleButton/ToggleButtonRow.tsx | Replaces child cloning with container-owned segmented styling + context provider. |
| src/components/ToggleButton/ToggleButton.tsx | Consumes row context to adjust styling for segmented rows. |
| src/components/List/ListItem.tsx | Applies indentation based on accordion context when no left/right slots are provided. |
| src/components/List/ListAccordionContext.tsx | Adds context used to coordinate indentation behavior for accordion descendants. |
| src/components/List/ListAccordion.tsx | Removes child cloning and provides indentation behavior via context. |
| src/components/Dialog/DialogTitle.tsx | Removes title’s own top offset to defer inset ownership to Dialog container. |
| src/components/Dialog/DialogIcon.tsx | Removes icon top padding, preserves icon-to-title spacing, adds testID. |
| src/components/Dialog/DialogActions.tsx | Stops injecting props/styles into action buttons; uses container spacing. |
| src/components/Dialog/Dialog.tsx | Removes first-child margin injection and moves top inset to dialog surface padding. |
| src/components/Card/utils.tsx | Simplifies cover styling helper signature after removing index/total injection. |
| src/components/Card/CardTitle.tsx | Removes internal positional props previously injected by Card. |
| src/components/Card/CardCover.tsx | Removes internal positional props and updates cover style calculation callsite. |
| src/components/Card/CardContent.tsx | Makes vertical padding uniform (no neighbor/position-based heuristics). |
| src/components/Card/CardActions.tsx | Stops injecting props into buttons and switches to container spacing. |
| src/components/Card/Card.tsx | Removes child cloning/indexing, clips content to card shape via overflow + radius. |
| src/components/tests/ToggleButton.test.tsx | Adds characterization test for segmented-row context styling. |
| src/components/tests/ListAccordion.test.tsx | Adds tests covering accordion child indentation via context. |
| src/components/tests/Dialog.test.tsx | Updates tests for surface-owned top inset + no action prop injection. |
| src/components/tests/Card/Card.test.tsx | Adds tests for clipping, uniform content padding, and no prop injection. |
| src/components/tests/Card/snapshots/Card.test.tsx.snap | Updates snapshot for inner-container clipping/radius styling. |
| example/src/Examples/TeamDetails.tsx | Updates examples for Card.Actions breaking change (explicit button mode). |
| example/src/Examples/CardExample.tsx | Updates examples for Card.Actions breaking change (explicit button mode). |
…factor - DialogActions: destructure `theme`/`style`/`children` and forward only the remaining ViewProps to the native View, so the custom `theme` prop is not spread onto it (avoids react-native-web warnings and prop leakage). - ToggleButton.Row: apply the `style` prop to the segmented row container instead of an extra wrapper View, so callers can override the row's background, border radius and padding again. - ToggleButton.Row: replace the flexbox `gap` divider with a per-segment hairline `marginLeft` (dropping the row's left padding), so segment dividers render on React Native versions without `gap` support; the peer dependency is `react-native: "*"`. Refs callstack#4989
CardActions/DialogActions still used flexbox `gap` for inter-action spacing, the same RN-version-unsafe pattern already flagged and fixed in ToggleButtonRow. Since react-native is declared as a `*` peer dependency, `gap` silently drops on older RN versions, removing spacing between actions. Replace with explicit layout-only spacer Views rendered between children — no gap dependency, no prop injection into the action elements. Appbar's medium/large title-row partition compared `child.type === AppbarContent` directly, so a React.memo-wrapped Appbar.Content was never recognized and landed in the controls row instead of the title row — contradicting the composition goal that wrapped children keep working. Unwrap memo's `.type` before comparing. Also fixes two existing Card/Dialog tests that asserted the styled `gap` value directly and indexed action children positionally; they now assert via testID, decoupled from spacer siblings changing child count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Several components inspect, classify, or mutate their children with
React.Children+cloneElement. This makes composition hard: children must be direct, prop-mergeable elements of an expected type, so wrapping them (in a memo, a custom component, a helper) silently breaks the injected styling/spacing/behavior, and the injected props are not type-safe. The FAB Menu rewrite (#4963) addressed the same class of problem; this PR migrates the remaining offenders to typed props, context, and container-owned layout.Approach
No child introspection. Each section component owns its own spacing/styling, cross-cutting state flows through context, and parents own layout (gap, padding, clipping) instead of cloning children. v6 alpha allows breaking changes without deprecation aliases, but user-visible output is preserved for documented usage; where a default that required child introspection is dropped, it is called out as a breaking change.
Changes
List.Accordion
React.Children.map+cloneElementthat injectedpaddingLeftandthemeinto expanded children.ListAccordionContext({ leftIndent }).List.Itemconsumes it and indents its own container when it renders noleft/right. Ripple/background stay full-width.List.Itemchildren (zero snapshot churn).Dialog
cloneElementthat injectedmarginTop: 24.paddingTop: 24);Dialog.TitleandDialog.Iconno longer add their own top offset.Dialog.Iconkeeps a 16px bottom gap so icon-to-title spacing is preserved.Dialog.Actions
compactanduppercaseinto action buttons. Set them explicitly on each button. Inter-action spacing is now containergap.Card
displayNamescanning, and the per-childcloneElementinjection ofindex/total/siblings/borderRadiusStyles.overflow: hidden+ combined border radius) instead of injecting corner styles intoCard.Cover/Card.Title.Card.Contentnow uses uniform vertical padding regardless of neighboring sections.Card.Actions
mode(previouslyoutlinedfor the first action,containedfor the rest) orcompact. Set button props explicitly. Spacing is containergap.ToggleButton.Row
Children.count/Children.map/cloneElementthat injected first/middle/last border radii into each button by index.ToggleButtonRowContext: the row clips a single rounded container and memberToggleButtons read the context flag to flatten their own corners, with hairline dividers between segments. Wrapped or conditionally-renderedToggleButtons compose correctly.main— samecorner.extraSmallcontainer shape, samegetToggleButtonColorselected fill, same outline divider. This is a pure composition refactor, not a restyle.themeprop toToggleButton.Rowso it can resolve the theme itself instead of relying on injection.Appbar
React.Children.forEachcount heuristic, thedisplayName-based child filtering, the action splitting, and thecloneElementinjection ofcolor/mode/themeintoAppbar.Action/Appbar.BackAction/Appbar.Content.AppbarContext({ isDark, mode }). Children read it and derive their own foreground color and title variant, instead of having props spliced in.smallandcenter-alignedrender their children directly in author order; the title'sflex: 1keeps trailing actions right-aligned and the center-aligned title centers itself.mediumandlargekeep the two-row layout (controls row above a full-width title) by partitioning children for placement only — no props are injected.Appbar.BackActionbeforeAppbar.Contentand trailingAppbar.Actions after it, as shown in the docs. Children no longer receive injectedcolor/mode/themeprops; wrappingAppbar.Content/Appbar.Actionin another element keeps working because the values come from context.Out of scope (kept — idiomatic, not composition-hostile)
Tooltipsingle-child trigger clone,Chipavatar-prop clone,TouchableRipple/TouchableRipple.nativeChildren.onlyvalidation.Breaking changes (summary)
BREAKING CHANGE:
Appbarrenders children in author order (no automatic back-action-first reordering) and no longer injectscolor/mode/theme— the shared values come from context; orderAppbar.BackActionbeforeAppbar.Contentas in the docs.BREAKING CHANGE:
Card.Actionsno longer injectsmode/compact— passmode="outlined"/mode="contained"(andcompactif needed) explicitly.BREAKING CHANGE:
Dialog.Actionsno longer injectscompact/uppercase— set them explicitly.BREAKING CHANGE:
Card.Contentuses uniform vertical padding regardless of neighbors.Example migration (Card.Actions):
Test plan
yarn typecheck— passyarn lint— passList.Accordion(context indent),Dialog(surface top spacing, icon-to-title gap, no action-prop injection),Card(clip-to-shape, uniformCard.Contentpadding, noCard.Actionsinjection),ToggleButton.Row(segmented styling via context) andAppbar(context-provided values, author-order rendering). The fullyarn testrun has pre-existing RNAnimatedfake-timer baseline failures unrelated to this change — the identical set fails onmain.main) vs after, on iOS (iPhone 16 Pro) and Android (Pixel 9 Pro XL): Appbar (small / center-aligned / medium / large), List.Accordion, Dialog, Card, and ToggleButton.Row — no visual regressions. Screenshots below.Visual verification
Manual before (
main) vs after comparison on iOS (iPhone 16 Pro) and Android (Pixel 9 Pro XL) — no visual regressions.Appbar — all four modes (Android)
Appbar — small + medium two-row (iOS)
Appbar — baseline vs after vs pixel diff (iOS, small): identical apart from the status-bar clock
List.Accordion · Dialog · ToggleButton.Row · Card — baseline (
main) vs after (Android)ToggleButton.Row + Card — base vs HEAD, zero pixel delta (Android)
Related: #4989
Reference: #4963