Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 2.2k
refactor: replace React.Children usage to more composition-friendly alternatives (#4989)#5018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Changes from all commits
00e72de0e91ea541ecff82d97fbc012f27f426827fd2f5f61414bc1317199dfb3d0b920d08f4638fb0b4File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -3,12 +3,8 @@ import { Animated, StyleSheet, View } from 'react-native'; | ||||||||||||||||||
| import type { ColorValue, StyleProp, ViewProps, ViewStyle } from 'react-native'; | ||||||||||||||||||
| import AppbarContent from './AppbarContent'; | ||||||||||||||||||
| import { | ||||||||||||||||||
| getAppbarBackgroundColor, | ||||||||||||||||||
| modeAppbarHeight, | ||||||||||||||||||
| renderAppbarContent, | ||||||||||||||||||
| filterAppbarActions, | ||||||||||||||||||
| } from './utils'; | ||||||||||||||||||
| import { AppbarContext } from './AppbarContext'; | ||||||||||||||||||
| import { getAppbarBackgroundColor, modeAppbarHeight } from './utils'; | ||||||||||||||||||
| import type { AppbarModes, AppbarChildProps } from './utils'; | ||||||||||||||||||
| import { useInternalTheme } from '../../core/theming'; | ||||||||||||||||||
| import type { Elevation, ThemeProp } from '../../types'; | ||||||||||||||||||
| @@ -174,45 +170,59 @@ const Appbar = ({ | ||||||||||||||||||
| const isDark = typeof dark === 'boolean' ? dark : false; | ||||||||||||||||||
| const isCenterAlignedMode = isMode('center-aligned'); | ||||||||||||||||||
| let shouldCenterContent = false; | ||||||||||||||||||
| let shouldAddLeftSpacing = false; | ||||||||||||||||||
| let shouldAddRightSpacing = false; | ||||||||||||||||||
| if (isCenterAlignedMode) { | ||||||||||||||||||
| let hasAppbarContent = false; | ||||||||||||||||||
| let leftItemsCount = 0; | ||||||||||||||||||
| let rightItemsCount = 0; | ||||||||||||||||||
| React.Children.forEach(children, (child) => { | ||||||||||||||||||
| if (React.isValidElement<AppbarChildProps>(child)) { | ||||||||||||||||||
| const isLeading = child.props.isLeading === true; | ||||||||||||||||||
| if (child.type === AppbarContent) { | ||||||||||||||||||
| hasAppbarContent = true; | ||||||||||||||||||
| } else if (isLeading || !hasAppbarContent) { | ||||||||||||||||||
| leftItemsCount++; | ||||||||||||||||||
| } else { | ||||||||||||||||||
| rightItemsCount++; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| }); | ||||||||||||||||||
| shouldCenterContent = | ||||||||||||||||||
| hasAppbarContent && leftItemsCount < 2 && rightItemsCount < 3; | ||||||||||||||||||
| shouldAddLeftSpacing = shouldCenterContent && leftItemsCount === 0; | ||||||||||||||||||
| shouldAddRightSpacing = shouldCenterContent && rightItemsCount === 0; | ||||||||||||||||||
| } | ||||||||||||||||||
| const spacingStyle = styles.v3Spacing; | ||||||||||||||||||
| const insets = { | ||||||||||||||||||
| paddingBottom: safeAreaInsets?.bottom, | ||||||||||||||||||
| paddingTop: safeAreaInsets?.top, | ||||||||||||||||||
| paddingLeft: safeAreaInsets?.left, | ||||||||||||||||||
| paddingRight: safeAreaInsets?.right, | ||||||||||||||||||
| }; | ||||||||||||||||||
| const appbarContextValue = React.useMemo( | ||||||||||||||||||
| () => ({ isDark, mode }), | ||||||||||||||||||
| [isDark, mode] | ||||||||||||||||||
| ); | ||||||||||||||||||
| let content: React.ReactNode = children; | ||||||||||||||||||
| if (isMode('medium') || isMode('large')) { | ||||||||||||||||||
| // Medium/large top app bars use a two-row layout: a controls row with the | ||||||||||||||||||
| // leading and trailing actions above a full-width title row. React Native | ||||||||||||||||||
| // flexbox has no `order`, so the title has to be separated from the actions | ||||||||||||||||||
| // structurally. We partition the children by element identity and the | ||||||||||||||||||
| // `isLeading` prop for layout only — nothing is injected into them; shared | ||||||||||||||||||
| // values flow through `AppbarContext`. | ||||||||||||||||||
| const items = React.Children.toArray(children).filter( | ||||||||||||||||||
| React.isValidElement | ||||||||||||||||||
| ) as React.ReactElement<AppbarChildProps>[]; | ||||||||||||||||||
| const isAppbarContent = (child: React.ReactElement<AppbarChildProps>) => { | ||||||||||||||||||
| const { type } = child; | ||||||||||||||||||
| // React.memo(AppbarContent) wraps the component in an object whose | ||||||||||||||||||
| // `.type` holds the original component — unwrap it so memoized | ||||||||||||||||||
| // Content still lands in the title row. | ||||||||||||||||||
| const innerType = | ||||||||||||||||||
| typeof type === 'object' && type !== null && 'type' in type | ||||||||||||||||||
| ? (type as { type: unknown }).type | ||||||||||||||||||
| : type; | ||||||||||||||||||
| return innerType === AppbarContent; | ||||||||||||||||||
| }; | ||||||||||||||||||
| const titleItems = items.filter(isAppbarContent); | ||||||||||||||||||
| const actionItems = items.filter((child) => !isAppbarContent(child)); | ||||||||||||||||||
| const leadingActions = actionItems.filter((child) => child.props.isLeading); | ||||||||||||||||||
| const trailingActions = actionItems.filter( | ||||||||||||||||||
| (child) => !child.props.isLeading | ||||||||||||||||||
| ); | ||||||||||||||||||
| content = ( | ||||||||||||||||||
| <View style={styles.columnContainer}> | ||||||||||||||||||
| <View style={styles.controlsRow}> | ||||||||||||||||||
| {leadingActions} | ||||||||||||||||||
| <View style={styles.rightActionControls}>{trailingActions}</View> | ||||||||||||||||||
| </View> | ||||||||||||||||||
| {titleItems} | ||||||||||||||||||
| </View> | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| return ( | ||||||||||||||||||
| <Surface | ||||||||||||||||||
| style={[ | ||||||||||||||||||
| @@ -228,77 +238,9 @@ const Appbar = ({ | ||||||||||||||||||
| container | ||||||||||||||||||
| {...rest} | ||||||||||||||||||
| > | ||||||||||||||||||
| {shouldAddLeftSpacing ? <View style={spacingStyle} /> : null} | ||||||||||||||||||
| {(isMode('small') || isMode('center-aligned')) && ( | ||||||||||||||||||
| <> | ||||||||||||||||||
| {/* Render only the back action at first place */} | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| children, | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| theme, | ||||||||||||||||||
| renderOnly: ['Appbar.BackAction'], | ||||||||||||||||||
| shouldCenterContent: isCenterAlignedMode || shouldCenterContent, | ||||||||||||||||||
| })} | ||||||||||||||||||
| {/* Render the rest of the content except the back action */} | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| // Filter appbar actions - first leading icons, then trailing icons | ||||||||||||||||||
| children: [ | ||||||||||||||||||
| ...filterAppbarActions(children, true), | ||||||||||||||||||
| ...filterAppbarActions(children), | ||||||||||||||||||
| ], | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| theme, | ||||||||||||||||||
| renderExcept: ['Appbar.BackAction'], | ||||||||||||||||||
| shouldCenterContent: isCenterAlignedMode || shouldCenterContent, | ||||||||||||||||||
| })} | ||||||||||||||||||
| </> | ||||||||||||||||||
| )} | ||||||||||||||||||
| {(isMode('medium') || isMode('large')) && ( | ||||||||||||||||||
| <View | ||||||||||||||||||
| style={[ | ||||||||||||||||||
| styles.columnContainer, | ||||||||||||||||||
| isMode('center-aligned') && styles.centerAlignedContainer, | ||||||||||||||||||
| ]} | ||||||||||||||||||
| > | ||||||||||||||||||
| {/* Appbar top row with controls */} | ||||||||||||||||||
| <View style={styles.controlsRow}> | ||||||||||||||||||
| {/* Left side of row container, can contain AppbarBackAction or AppbarAction if it's leading icon */} | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| children, | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| renderOnly: ['Appbar.BackAction'], | ||||||||||||||||||
| mode, | ||||||||||||||||||
| })} | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| children: filterAppbarActions(children, true), | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| renderOnly: ['Appbar.Action'], | ||||||||||||||||||
| mode, | ||||||||||||||||||
| })} | ||||||||||||||||||
| {/* Right side of row container, can contain other AppbarAction if they are not leading icons */} | ||||||||||||||||||
| <View style={styles.rightActionControls}> | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| children: filterAppbarActions(children), | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| renderExcept: [ | ||||||||||||||||||
| 'Appbar', | ||||||||||||||||||
| 'Appbar.BackAction', | ||||||||||||||||||
| 'Appbar.Content', | ||||||||||||||||||
| 'Appbar.Header', | ||||||||||||||||||
| ], | ||||||||||||||||||
| mode, | ||||||||||||||||||
| })} | ||||||||||||||||||
| </View> | ||||||||||||||||||
| </View> | ||||||||||||||||||
| {renderAppbarContent({ | ||||||||||||||||||
| children, | ||||||||||||||||||
| isDark, | ||||||||||||||||||
| renderOnly: ['Appbar.Content'], | ||||||||||||||||||
| mode, | ||||||||||||||||||
| })} | ||||||||||||||||||
| </View> | ||||||||||||||||||
| )} | ||||||||||||||||||
| {shouldAddRightSpacing ? <View style={spacingStyle} /> : null} | ||||||||||||||||||
| <AppbarContext.Provider value={appbarContextValue}> | ||||||||||||||||||
| {content} | ||||||||||||||||||
| </AppbarContext.Provider> | ||||||||||||||||||
Comment on lines
+241
to
+243
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should keep
Suggested change
| ||||||||||||||||||
| </Surface> | ||||||||||||||||||
| ); | ||||||||||||||||||
| }; | ||||||||||||||||||
| @@ -309,9 +251,6 @@ const styles = StyleSheet.create({ | ||||||||||||||||||
| alignItems: 'center', | ||||||||||||||||||
| paddingHorizontal: 4, | ||||||||||||||||||
| }, | ||||||||||||||||||
| v3Spacing: { | ||||||||||||||||||
| width: 52, | ||||||||||||||||||
| }, | ||||||||||||||||||
| controlsRow: { | ||||||||||||||||||
| flex: 1, | ||||||||||||||||||
| flexDirection: 'row', | ||||||||||||||||||
| @@ -328,9 +267,6 @@ const styles = StyleSheet.create({ | ||||||||||||||||||
| flex: 1, | ||||||||||||||||||
| paddingTop: 8, | ||||||||||||||||||
| }, | ||||||||||||||||||
| centerAlignedContainer: { | ||||||||||||||||||
| paddingTop: 0, | ||||||||||||||||||
| }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| export default Appbar; | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import * as React from 'react'; | ||
| import type { AppbarModes } from './utils'; | ||
| export type AppbarContextType = { | ||
| /** | ||
| * Whether the Appbar background is dark, so children can derive a | ||
| * contrasting default foreground color without it being injected. | ||
| */ | ||
| isDark: boolean; | ||
| /** | ||
| * The Appbar mode, consumed by `Appbar.Content` to pick the title text | ||
| * variant and container layout. | ||
| */ | ||
| mode: AppbarModes; | ||
| }; | ||
| /** | ||
| * Shared Appbar values provided to `Appbar.Action`, `Appbar.BackAction` and | ||
| * `Appbar.Content` via context instead of being injected with `cloneElement`. | ||
| * This keeps composition intact: children can be wrapped, reordered or | ||
| * conditionally rendered without losing the values they need. | ||
| */ | ||
| export const AppbarContext = React.createContext<AppbarContextType | null>( | ||
| null | ||
| ); | ||
| export const useAppbarContext = () => React.useContext(AppbarContext); |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about replacing type-based partition with explicit
leading/title/actionsslots?Children.toArraycan't see custom wrapper’s output, so<WrappedContent />is still placed in trailing actionsso, we could add these props:
simplify this whole block:
and usage then might be smth like that:
but that's a breaking API change for medium/large
Appbars, so all existing usages & docs would need migration