refactor: replace React.Children usage to more composition-friendly alternatives (#4989)#5018
Draft
matkoson wants to merge 6 commits into
Draft
refactor: replace React.Children usage to more composition-friendly alternatives (#4989)#5018matkoson wants to merge 6 commits into
matkoson wants to merge 6 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.
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/cloneElementfirst/middle/last border injection.ToggleButtonRowContext; the row is an MD3 segmented container (outline-colored background with hairline dividers and padding, clipped corners), and memberToggleButtons flatten their corners via context. WrappedToggleButtons compose correctly.themeprop toToggleButton.Row.Out of scope (kept — idiomatic, not composition-hostile)
Tooltipsingle-child trigger clone,Chipavatar-prop clone,TouchableRipple/TouchableRipple.nativeChildren.onlyvalidation.Deferred (follow-up PR)
Appbar+Appbar/utilsstill use child counting,displayNamefiltering, action splitting, andcloneElementinjection. Targeted for a separate change (context + explicit author ordering).Breaking changes (summary)
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.BREAKING CHANGE:
ToggleButton.Rowsegmented styling is reworked to an MD3 container (visual refresh).Example migration (Card.Actions):
Test plan
yarn typecheck— passyarn lint— pass (one pre-existing, unrelated TextInput warning)yarn test ListAccordion(12) — indent applied/not-applied via contextyarn test Dialog(12) — surface top spacing for title/content/icon-first, icon-to-title spacing, no action-prop injectionyarn test Cardscoped (20) — clip to card shape, uniformCard.Contentpadding, noCard.Actionsprop injectionyarn test ToggleButtonscoped (7) — segmented styling via contextyarn testsuite has pre-existing animation/native-view baseline failures unrelated to this change (identical set onmain); scoped runs are green.main) vs after, on iOS (iPhone 16 Pro) and Android (Pixel 9 Pro XL): List.Accordion pixel-identical; Dialog icon-to-title spacing preserved; ToggleButton.Row segmented appearance preserved; Card reflects the intended uniform-padding change. No regressions.Related: #4989
Reference: #4963