diff --git a/docs/prop-patterns.md b/docs/prop-patterns.md new file mode 100644 index 00000000..96d32d6b --- /dev/null +++ b/docs/prop-patterns.md @@ -0,0 +1,129 @@ +# Component Prop Patterns + +This document explains the prop-passing conventions used in TeachLink mobile and why they exist. + +--- + +## The Problem with `{...props}` / `{...rest}` Spreading + +Spreading all props blindly onto a child element has several downsides: + +1. **Unnecessary re-renders** — any prop change in the bag, including ones the component doesn't use, triggers a re-render of the child. +2. **Hidden contracts** — callers can't tell from the interface which props are safe to pass without reading the implementation. +3. **Prop pollution** — non-standard props may silently land on native elements and produce React warnings. + +--- + +## Rule: Always Use Explicit Prop Destructuring in Wrapper Components + +### ✅ Do this + +```tsx +// Good: explicit contract, predictable rendering +interface MyButtonProps { + label: string; + onPress: () => void; + testID?: string; + disabled?: boolean; +} + +export const MyButton: React.FC = ({ + label, + onPress, + testID, + disabled, +}) => ( + + {label} + +); +``` + +### ❌ Don't do this + +```tsx +// Bad: opaque contract, unnecessary re-renders +interface MyButtonProps extends TouchableOpacityProps { + label: string; +} + +export const MyButton: React.FC = ({ label, ...rest }) => ( + + {label} + +); +``` + +--- + +## When Spreading Is Acceptable + +There are two legitimate exceptions to the above rule: + +### 1. Higher-Order Components (HOC) + +A HOC wraps an *unknown* component type and must forward **all** of its props. The spread is unavoidable here because the wrapped component's props are not known at compile time. + +```tsx +// AuthGuard.tsx — must forward every prop of the wrapped component +export function withAuthGuard

( + Component: React.ComponentType

, +): React.ComponentType

{ + return function AuthGuardedComponent(props: P) { + return ( + + + + ); + }; +} +``` + +Similarly, `LazyScreen.tsx` uses `{...props}` because it wraps a lazily-loaded, unknown component type. + +### 2. React Navigation Drawer Internals + +`MobileDrawer` must forward the entire `DrawerContentComponentProps` bag to `` and `` because React Navigation requires this exact shape for navigation to function. + +### 3. Computed Internal Objects + +Spreading a **locally-computed** object (not external props) is fine, because the object's shape is known and controlled: + +```tsx +// PullToRefresh.tsx — spreading internal PanResponder handlers + +``` + +These computed-object spreads are not the same as forwarding unknown external props. + +--- + +## Adding New Props to a Wrapper Component + +1. Add the new prop to the component's **explicit interface** with a JSDoc comment. +2. Destructure it from the function parameters. +3. Pass it by name to the underlying element. +4. Do **not** fall back to `...rest` — keep the list explicit. + +--- + +## Files That Were Refactored (Issue #371) + +| Component | Change | +|---|---| +| `AppText.tsx` | Replaced `{...props}` with explicit `TextProps` subset | +| `AccessibleButton.tsx` | Replaced `{...rest}` with explicit `TouchableOpacityProps` subset | +| `MobileFormInput.tsx` | Replaced `{...rest}` with explicit `TextInputProps` subset | +| `InfiniteVirtualList.tsx` | Replaced `{...rest}` with explicit `FlatListProps` extension | +| `VirtualList.tsx` | Replaced `{...rest}` with explicit `FlatListProps` extension | + +Files where spreading was **intentionally kept** (see exceptions above): + +| File | Reason | +|---|---| +| `AuthGuard.tsx` | HOC pattern — must forward all props of wrapped component | +| `LazyScreen.tsx` | Generic lazy-loader HOC — same as above | +| `MobileDrawer.tsx` | React Navigation requires full `DrawerContentComponentProps` forwarding | +| `PullToRefresh.tsx` | Internal computed objects (PanResponder handlers, scroll props) | +| `MobileVideoPlayer.tsx` | Internal computed PanResponder handlers | +| `AchievementBadges.tsx` | Computed accessibility object spread — not external prop forwarding | diff --git a/src/components/common/AppText.tsx b/src/components/common/AppText.tsx index abfca2fd..47da037a 100644 --- a/src/components/common/AppText.tsx +++ b/src/components/common/AppText.tsx @@ -1,20 +1,76 @@ import React from 'react'; -import { Text as RNText, TextProps, StyleSheet } from 'react-native'; +import { Text as RNText, StyleSheet } from 'react-native'; +import type { + StyleProp, + TextStyle, + AccessibilityRole, + AccessibilityState, +} from 'react-native'; import { useDynamicFontSize } from '../../hooks'; -interface AppTextProps extends TextProps { +/** + * Explicit subset of React Native TextProps that AppText consumers may pass. + * + * NOTE: Do NOT add `{...rest}` or generic prop spreading here. + * If you need an additional prop, add it explicitly to this interface + * and thread it through to below. This keeps the component + * surface area predictable and prevents unnecessary re-renders caused + * by unknown prop changes. See docs/prop-patterns.md. + */ +export interface AppTextProps { + /** Text content */ + children?: React.ReactNode; + /** Style overrides for the text element */ + style?: StyleProp; /** * If true, the font size will NOT be scaled. * Useful for elements that should remain at a fixed size regardless of system settings. */ fixed?: boolean; + /** Maximum number of lines before truncation */ + numberOfLines?: number; + /** Truncation strategy when numberOfLines is set */ + ellipsizeMode?: 'head' | 'middle' | 'tail' | 'clip'; + /** Press handler */ + onPress?: () => void; + /** Long-press handler */ + onLongPress?: () => void; + /** Test identifier for automated tests */ + testID?: string; + /** Accessibility label for screen readers */ + accessibilityLabel?: string; + /** Accessibility role for screen readers */ + accessibilityRole?: AccessibilityRole; + /** Accessibility hint for screen readers */ + accessibilityHint?: string; + /** Accessibility state for screen readers */ + accessibilityState?: AccessibilityState; + /** Controls importance for accessibility on Android */ + importantForAccessibility?: 'auto' | 'yes' | 'no' | 'no-hide-descendants'; + /** Allow the text to be selected by the user */ + selectable?: boolean; } /** * A wrapper around React Native's Text component that uses the useDynamicFontSize hook * to ensure consistent scaling across the application. */ -export const AppText: React.FC = ({ style, fixed = false, ...props }) => { +export const AppText: React.FC = ({ + style, + fixed = false, + children, + numberOfLines, + ellipsizeMode, + onPress, + onLongPress, + testID, + accessibilityLabel, + accessibilityRole, + accessibilityHint, + accessibilityState, + importantForAccessibility, + selectable, +}) => { const { scale } = useDynamicFontSize(); // We flatten the style to easily extract and modify the fontSize @@ -33,11 +89,23 @@ export const AppText: React.FC = ({ style, fixed = false, ...props return ( + numberOfLines={numberOfLines} + ellipsizeMode={ellipsizeMode} + onPress={onPress} + onLongPress={onLongPress} + testID={testID} + accessibilityLabel={accessibilityLabel} + accessibilityRole={accessibilityRole} + accessibilityHint={accessibilityHint} + accessibilityState={accessibilityState} + importantForAccessibility={importantForAccessibility} + selectable={selectable} + > + {children} + ); }; diff --git a/src/components/mobile/AccessibleButton.tsx b/src/components/mobile/AccessibleButton.tsx index 21df836c..a56354da 100644 --- a/src/components/mobile/AccessibleButton.tsx +++ b/src/components/mobile/AccessibleButton.tsx @@ -1,32 +1,35 @@ import React from 'react'; import { TouchableOpacity, - TouchableOpacityProps, + StyleSheet, StyleProp, ViewStyle, + GestureResponderEvent, } from 'react-native'; import { getAccessibilityProps } from '../../utils/accessibility'; /** - * Props for the AccessibleButton component + * Explicit props for the AccessibleButton component. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * If you need an additional TouchableOpacity prop, add it explicitly to this + * interface and thread it through to below. + * See docs/prop-patterns.md. */ -interface AccessibleButtonProps extends TouchableOpacityProps { - /** Accessibility label for screen readers */ +interface AccessibleButtonProps { label: string; - /** Additional accessibility hint for screen readers */ hint?: string; - /** Accessibility role for the button */ role?: 'button' | 'link'; - /** Optional custom styles for the button container */ containerStyle?: StyleProp; - /** Optional NativeWind className */ - className?: string; + style?: StyleProp; + children?: React.ReactNode; + disabled?: boolean; + activeOpacity?: number; + onPress?: (event: GestureResponderEvent) => void; + onLongPress?: (event: GestureResponderEvent) => void; + testID?: string; } -/** - * A reusable accessible button component for TeachLink mobile. - * Ensures a minimum touch target of 44x44 and provides consistent accessibility props. - */ export const AccessibleButton: React.FC = ({ label, hint, @@ -36,24 +39,34 @@ export const AccessibleButton: React.FC = ({ containerStyle, disabled, activeOpacity = 0.7, - className, - ...rest -}: AccessibleButtonProps) => { + onPress, + onLongPress, + testID, +}) => { const accessibilityProps = getAccessibilityProps(label, role as 'button' | 'link', hint, { disabled: !!disabled, }); return ( {children} ); }; +const styles = StyleSheet.create({ + base: { + minWidth: 44, + minHeight: 44, + justifyContent: 'center', + alignItems: 'center', + }, +}); \ No newline at end of file diff --git a/src/components/mobile/InfiniteVirtualList.tsx b/src/components/mobile/InfiniteVirtualList.tsx index 80ecfd71..28858ef5 100644 --- a/src/components/mobile/InfiniteVirtualList.tsx +++ b/src/components/mobile/InfiniteVirtualList.tsx @@ -12,6 +12,13 @@ import { import * as Device from 'expo-device'; import { useMemoryMonitor } from '../../hooks'; +/** + * Explicit extension props for InfiniteVirtualList. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * All supported FlatList extension points must be listed explicitly. + * See docs/prop-patterns.md. + */ export interface InfiniteVirtualListProps extends Omit, 'renderItem'> { /** The data items to display. */ data: ReadonlyArray | null | undefined; @@ -33,6 +40,24 @@ export interface InfiniteVirtualListProps extends Omit, 'ren style?: StyleProp; /** Custom styles for the inner scroll container. */ contentContainerStyle?: StyleProp; + /** Component rendered above the list items. */ + ListHeaderComponent?: FlatListProps['ListHeaderComponent']; + /** Component rendered when the list is empty. */ + ListEmptyComponent?: FlatListProps['ListEmptyComponent']; + /** Render horizontally instead of vertically. */ + horizontal?: boolean; + /** Hide the vertical scroll indicator. */ + showsVerticalScrollIndicator?: boolean; + /** Hide the horizontal scroll indicator. */ + showsHorizontalScrollIndicator?: boolean; + /** Pull-to-refresh control. */ + refreshControl?: FlatListProps['refreshControl']; + /** Scroll event throttle in milliseconds. */ + scrollEventThrottle?: number; + /** Scroll event callback. */ + onScroll?: FlatListProps['onScroll']; + /** Test identifier for automated tests. */ + testID?: string; } /** @@ -51,7 +76,15 @@ export function InfiniteVirtualList({ listId = 'InfiniteVirtualList', style, contentContainerStyle, - ...rest + ListHeaderComponent, + ListEmptyComponent, + horizontal, + showsVerticalScrollIndicator, + showsHorizontalScrollIndicator, + refreshControl, + scrollEventThrottle, + onScroll, + testID, }: InfiniteVirtualListProps) { // Monitor JS collection array sizes useMemoryMonitor({ @@ -112,12 +145,20 @@ export function InfiniteVirtualList({ onEndReached={onEndReached} onEndReachedThreshold={onEndReachedThreshold} ListFooterComponent={renderFooter} + ListHeaderComponent={ListHeaderComponent} + ListEmptyComponent={ListEmptyComponent} // Performance configurations: removeClippedSubviews={true} // Free native memory by unmounting offscreen views style={style} contentContainerStyle={contentContainerStyle} + horizontal={horizontal} + showsVerticalScrollIndicator={showsVerticalScrollIndicator} + showsHorizontalScrollIndicator={showsHorizontalScrollIndicator} + refreshControl={refreshControl} + scrollEventThrottle={scrollEventThrottle} + onScroll={onScroll} + testID={testID} {...optimizations} - {...rest} /> ); } diff --git a/src/components/mobile/MobileDrawer.tsx b/src/components/mobile/MobileDrawer.tsx index 06ae07b2..f4648712 100644 --- a/src/components/mobile/MobileDrawer.tsx +++ b/src/components/mobile/MobileDrawer.tsx @@ -1,17 +1,18 @@ -import React from 'react'; -import { View, Text, TouchableOpacity } from 'react-native'; import { DrawerContentComponentProps, DrawerContentScrollView, DrawerItemList, } from '@react-navigation/drawer'; -import { useSafeArea } from '../../hooks'; import { Settings, LogOut, Sun, Moon } from 'lucide-react-native'; +import React from 'react'; +import { View, Text, TouchableOpacity } from 'react-native'; + +import { useSafeArea } from '../../hooks'; /** * Props for the MobileDrawer component */ -export const MobileDrawer = (props: DrawerContentComponentProps) => { +export const MobileDrawer = ({ state, navigation, descriptors }: DrawerContentComponentProps) => { const { top, bottom } = useSafeArea(); const [isDark, setIsDark] = React.useState(false); @@ -41,8 +42,13 @@ export const MobileDrawer = (props: DrawerContentComponentProps) => { - - + + void; - /** Error message to display */ + placeholder?: string; error?: string; - /** Hint text to display next to the label */ hint?: string; - /** Icon to display on the left side of the input */ leftIcon?: React.ReactNode; - /** Whether the field is required */ required?: boolean; - /** Whether to use dark mode styling */ isDark?: boolean; - /** Shared cache key for autofill and suggestions */ + secureTextEntry?: boolean; + multiline?: boolean; + keyboardType?: KeyboardTypeOptions; + autoCapitalize?: AutoCapitalize; + autoCorrect?: boolean; + autoComplete?: React.ComponentProps['autoComplete']; + returnKeyType?: ReturnKeyTypeOptions; + onSubmitEditing?: (event: NativeSyntheticEvent) => void; + maxLength?: number; + editable?: boolean; + testID?: string; + accessibilityLabel?: string; cacheKey?: FormCacheFieldKey; - /** Persist value to cache on blur (default: true when cacheKey is set) */ cacheOnBlur?: boolean; - /** Optional ref for focusing the underlying input from parent screens */ inputRef?: React.Ref; + onBlur?: TextInputProps['onBlur']; } export const MobileFormInput: React.FC = ({ @@ -48,14 +58,22 @@ export const MobileFormInput: React.FC = ({ leftIcon, required = false, isDark = false, - cacheKey, - cacheOnBlur = true, - inputRef, secureTextEntry, multiline = false, keyboardType = 'default', + autoCapitalize, + autoCorrect, + autoComplete, + returnKeyType, + onSubmitEditing, + maxLength, + editable, + testID, + accessibilityLabel, + cacheKey, + cacheOnBlur = true, + inputRef, onBlur, - ...rest }) => { const [isFocused, setIsFocused] = useState(false); const [showPassword, setShowPassword] = useState(false); @@ -101,7 +119,6 @@ export const MobileFormInput: React.FC = ({ const handleTogglePassword = useCallback(() => setShowPassword(prev => !prev), []); const borderColor = error ? '#ef4444' : isFocused ? '#19c3e6' : isDark ? '#334155' : '#e2e8f0'; - const labelColor = error ? '#ef4444' : isDark ? '#94a3b8' : '#64748b'; return ( @@ -155,7 +172,15 @@ export const MobileFormInput: React.FC = ({ secureTextEntry={isPassword && !showPassword} multiline={multiline} keyboardType={keyboardType} - {...rest} + autoCapitalize={autoCapitalize} + autoCorrect={autoCorrect} + autoComplete={autoComplete} + returnKeyType={returnKeyType} + onSubmitEditing={onSubmitEditing} + maxLength={maxLength} + editable={editable} + testID={testID} + accessibilityLabel={accessibilityLabel} /> {isPassword && ( @@ -206,5 +231,4 @@ export const MobileFormInput: React.FC = ({ )} ); -}; - +}; \ No newline at end of file diff --git a/src/components/mobile/OfflineIndicator.tsx b/src/components/mobile/OfflineIndicator.tsx index 6e8a17ff..a60910b7 100644 --- a/src/components/mobile/OfflineIndicator.tsx +++ b/src/components/mobile/OfflineIndicator.tsx @@ -1,7 +1,8 @@ import React from 'react'; import { View, Text, TouchableOpacity } from 'react-native'; + import { useNetworkStatus } from '../../hooks'; -import logger from '../../utils/logger'; +import defaultLogger from '../../utils/logger'; // Props interface interface OfflineIndicatorProps { @@ -24,7 +25,7 @@ export const OfflineIndicator: React.FC = ({ onPress = () => {}, showDetails = false, }) => { - const { isOnline, isOffline, networkStatus, refresh } = useNetworkStatus(); + const { isOnline, isOffline, refresh } = useNetworkStatus(); // Don't show when online unless explicitly requested if (isOnline && !showWhenOnline) { @@ -62,7 +63,7 @@ export const OfflineIndicator: React.FC = ({ onPress(); } } catch (error) { - logger.error('Error refreshing network status:', error); + defaultLogger.error('Error refreshing network status:', error); } }; @@ -94,14 +95,35 @@ export const OfflineIndicator: React.FC = ({ // Export simplified versions export const OfflineBanner = OfflineIndicator; -export const OnlineIndicator = (props: any) => +export const OnlineIndicator = ({ + position = 'top', + backgroundColor = '#4CAF50', + textColor, + onPress, + showDetails, +}: OfflineIndicatorProps) => React.createElement(OfflineIndicator, { - ...props, + position, + backgroundColor, + textColor, + onPress, + showDetails, showWhenOnline: true, - backgroundColor: '#4CAF50', }); -export const ConnectionQualityIndicator = (props: any) => - React.createElement(OfflineIndicator, { ...props, position: 'bottom' }); -export const OfflineFAB = (props: any) => null; // Disabled due to configuration issues +export const ConnectionQualityIndicator = ({ + position = 'bottom', + backgroundColor = '#FF5722', + textColor, + onPress, + showDetails, +}: OfflineIndicatorProps) => + React.createElement(OfflineIndicator, { + position, + backgroundColor, + textColor, + onPress, + showDetails, + }); +export const OfflineFAB = (_props: OfflineIndicatorProps) => null; // Disabled due to configuration issues export default OfflineIndicator; diff --git a/src/components/mobile/VirtualList.tsx b/src/components/mobile/VirtualList.tsx index 9e222c20..f0b90cac 100644 --- a/src/components/mobile/VirtualList.tsx +++ b/src/components/mobile/VirtualList.tsx @@ -2,13 +2,35 @@ import React, { useCallback } from 'react'; import { FlatList, FlatListProps, ViewStyle, StyleProp } from 'react-native'; import { useMemoryMonitor } from '../../hooks'; +/** + * Explicit extension props for VirtualList. + * + * NOTE: Do NOT reintroduce `{...rest}` or generic prop spreading here. + * All supported FlatList extension points must be listed explicitly. + * See docs/prop-patterns.md. + */ + export interface VirtualListProps extends Omit, 'renderItem'> { data: ReadonlyArray | null | undefined; renderItem: FlatListProps['renderItem']; keyExtractor: (item: T, index: number) => string; - itemHeight?: number; // Optional: If items have fixed height, this drastically improves layout performance + /** Optional: If items have fixed height, this drastically improves layout performance */ + itemHeight?: number; contentContainerStyle?: StyleProp; - listId?: string; // Optional ID for memory monitoring + /** Optional ID for memory monitoring */ + listId?: string; + /** Component rendered above the list items. */ + ListHeaderComponent?: FlatListProps['ListHeaderComponent']; + /** Component rendered when the list is empty. */ + ListEmptyComponent?: FlatListProps['ListEmptyComponent']; + /** Render horizontally instead of vertically. */ + horizontal?: boolean; + /** Hide the vertical scroll indicator. */ + showsVerticalScrollIndicator?: boolean; + /** Pull-to-refresh control. */ + refreshControl?: FlatListProps['refreshControl']; + /** Test identifier for automated tests. */ + testID?: string; } /** @@ -21,7 +43,13 @@ export function VirtualList({ keyExtractor, itemHeight, listId = 'VirtualList', - ...rest + contentContainerStyle, + ListHeaderComponent, + ListEmptyComponent, + horizontal, + showsVerticalScrollIndicator, + refreshControl, + testID, }: VirtualListProps) { // Monitor memory footprint based on list size useMemoryMonitor({ @@ -49,7 +77,13 @@ export function VirtualList({ maxToRenderPerBatch={10} // Reduce number of items rendered per batch windowSize={5} // Reduce the size of the render window (default 21) updateCellsBatchingPeriod={50} // Delay in ms between batch renders - {...rest} + contentContainerStyle={contentContainerStyle} + ListHeaderComponent={ListHeaderComponent} + ListEmptyComponent={ListEmptyComponent} + horizontal={horizontal} + showsVerticalScrollIndicator={showsVerticalScrollIndicator} + refreshControl={refreshControl} + testID={testID} /> ); }