Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 129 additions & 0 deletions docs/prop-patterns.md
Original file line number Diff line number Diff line change
@@ -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<MyButtonProps> = ({
label,
onPress,
testID,
disabled,
}) => (
<TouchableOpacity onPress={onPress} testID={testID} disabled={disabled}>
<Text>{label}</Text>
</TouchableOpacity>
);
```

### ❌ Don't do this

```tsx
// Bad: opaque contract, unnecessary re-renders
interface MyButtonProps extends TouchableOpacityProps {
label: string;
}

export const MyButton: React.FC<MyButtonProps> = ({ label, ...rest }) => (
<TouchableOpacity {...rest}>
<Text>{label}</Text>
</TouchableOpacity>
);
```

---

## 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<P extends object>(
Component: React.ComponentType<P>,
): React.ComponentType<P> {
return function AuthGuardedComponent(props: P) {
return (
<AuthGuard>
<Component {...props} />
</AuthGuard>
);
};
}
```

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 `<DrawerContentScrollView>` and `<DrawerItemList>` 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
<View {...responderHandlers}>
```

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 |
78 changes: 73 additions & 5 deletions src/components/common/AppText.tsx
Original file line number Diff line number Diff line change
@@ -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 <RNText> 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<TextStyle>;
/**
* 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<AppTextProps> = ({ style, fixed = false, ...props }) => {
export const AppText: React.FC<AppTextProps> = ({
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
Expand All @@ -33,11 +89,23 @@ export const AppText: React.FC<AppTextProps> = ({ style, fixed = false, ...props

return (
<RNText
{...props}
style={dynamicStyle}
// We set allowFontScaling to false because we are manually scaling
// via the dynamicStyle to have explicit control via our hook.
allowFontScaling={false}
/>
numberOfLines={numberOfLines}
ellipsizeMode={ellipsizeMode}
onPress={onPress}
onLongPress={onLongPress}
testID={testID}
accessibilityLabel={accessibilityLabel}
accessibilityRole={accessibilityRole}
accessibilityHint={accessibilityHint}
accessibilityState={accessibilityState}
importantForAccessibility={importantForAccessibility}
selectable={selectable}
>
{children}
</RNText>
);
};
53 changes: 33 additions & 20 deletions src/components/mobile/AccessibleButton.tsx
Original file line number Diff line number Diff line change
@@ -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 <TouchableOpacity> 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<ViewStyle>;
/** Optional NativeWind className */
className?: string;
style?: StyleProp<ViewStyle>;
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<AccessibleButtonProps> = ({
label,
hint,
Expand All @@ -36,24 +39,34 @@ export const AccessibleButton: React.FC<AccessibleButtonProps> = ({
containerStyle,
disabled,
activeOpacity = 0.7,
className,
...rest
}: AccessibleButtonProps) => {
onPress,
onLongPress,
testID,
}) => {
const accessibilityProps = getAccessibilityProps(label, role as 'button' | 'link', hint, {
disabled: !!disabled,
});

return (
<TouchableOpacity
{...rest}
{...accessibilityProps}
onPress={onPress}
onLongPress={onLongPress}
disabled={disabled}
activeOpacity={activeOpacity}
className={`min-touch-target justify-center items-center ${className || ''}`}
style={[containerStyle, style]}
testID={testID}
style={[styles.base, containerStyle, style]}
{...accessibilityProps}
>
{children}
</TouchableOpacity>
);
};

const styles = StyleSheet.create({
base: {
minWidth: 44,
minHeight: 44,
justifyContent: 'center',
alignItems: 'center',
},
});
Loading
Loading