refactor: replace React.Children usage with context and typed props - #5076
Open
adam-sajko wants to merge 18 commits into
Open
refactor: replace React.Children usage with context and typed props#5076adam-sajko wants to merge 18 commits into
adam-sajko wants to merge 18 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.
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.
…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.
adam-sajko
changed the title
4 tasks
adam-sajko
marked this pull request as ready for review
August 27, 2026 11:18
callstack#4934 proposes an `actions` prop API for Appbar and callstack#5075 implements it, so the children refactor here is superseded. Appbar sources, tests and snapshots go back to their state on main.
Appbar is out of this PR. List.Accordion used to indent any child without its own left or right element, that indent now comes from context and only List.Item reads it.
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
2 participants
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
Continues #5018 by @matkoson, rebased on
mainwith the review feedback applied.React.Childrentogether withcloneElementlocks consumers into a fixed child shape. A child wrapped in anything, aTooltip, a layoutView, or a component of your own, silently stops receiving the props the parent meant to inject. This replaces the pattern with context, typed props and container-owned layout in four components.Cardno longer walks its children to pick padding or to assignmodeandcompactto action buttons.Card.Contentuses one vertical padding andCard.Actionsonly spaces the items.Dialogowned the top inset by cloning its first child withmarginTop: 24. The container now carriespaddingTopinstead.Dialog.Actionsno longer injectscompactanduppercase.List.Accordionpasses the indent through a newListAccordionContextinstead of cloning children with astyle.ToggleButton.Rowpasses the segmented flag and button position through a newToggleButtonRowContextinstead of cloning children.Eight
React.ChildrenandcloneElementcall sites are gone. What is left inCard.ActionsandDialog.Actionsis atoArraythat only inserts spacing between items, so nothing is injected any more.Breaking changes are in
docs/6.x/docs/guides/migration.md.Appbar is deliberately out of scope here. #4934 proposes an
actionsprop API and #5075 implements it, so a children refactor in Appbar would be thrown away. The Appbar files in this branch were reverted tomain.Related issue
#4989, continues #5018
Test plan
yarn test,yarn lintandyarn typecheckpass. 55 suites, 747 tests, 169 snapshots.New and updated tests cover the removed injection:
Card.test.tsxandDialog.test.tsxassert that action buttons keep the props the consumer set,ListAccordion.test.tsxcovers the context indent,ToggleButton.test.tsxcovers the segmented row.In the example app,
CardExampleandTeamDetailsnow setmodeon the buttons insideCard.Actions, since it is no longer assigned for them.One unrelated change rides along.
Tooltip.test.tsxhas a one line timing fix for a flake that trips the pre-commit hook onmain. It can be moved out into its own PR (please ask if you want it)