-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Description
Describe the bug
The ButtonBase component extends TouchableOpacityProps, which includes a disabled prop. However, the component defines and uses its own isDisabled prop. This creates a confusing API surface where:
- TypeScript allows passing both
disabled(inherited fromTouchableOpacityProps) andisDisabled(custom prop) - Only
isDisabledis properly handled by the component logic - If someone passes
disabledinstead ofisDisabled, it may not work as expected due to the explicitdisabled={isDisabled}mapping on line 45
Affected Component: app/component-library/components/Buttons/Button/foundation/ButtonBase
Code References:
ButtonBase.types.ts:12- Interface extendsTouchableOpacityPropsButtonBase.tsx:33-isDisabledprop is destructuredButtonBase.tsx:45-disabled={isDisabled}explicitly maps the custom propButtonBase.tsx:51-{...props}spreads remaining props, potentially including inheriteddisabled
Expected behavior
The component should have a clear, unambiguous API that only accepts isDisabled and explicitly omits the disabled prop from TouchableOpacityProps to prevent confusion and potential bugs.
Steps to reproduce
- Open
app/component-library/components/Buttons/Button/foundation/ButtonBase/ButtonBase.types.ts - Note that
ButtonBaseProps extends TouchableOpacityProps(line 12) - See that both
disabled(from TouchableOpacity) andisDisabled(custom) are technically valid props - Check
ButtonBase.tsx:45where onlyisDisabledis used:disabled={isDisabled} - Attempt to use
<ButtonBase disabled={true} />- TypeScript won't complain but it may not work correctly
Proposed Solution
Explicitly omit the disabled prop from TouchableOpacityProps:
export interface ButtonBaseProps extends Omit<TouchableOpacityProps, 'disabled'> {
// ... existing props
isDisabled?: boolean;
}This ensures:
- Only
isDisabledis accepted by the component API - TypeScript will error if someone tries to use
disabled - The API is clear and unambiguous
Where was this bug found?
Internal code review
Additional context
This is a technical debt issue that affects API clarity and could lead to bugs if developers mistakenly use disabled instead of isDisabled. While the component is marked as deprecated in favor of @metamask/design-system-react-native, it's still used in the codebase and should have a clear API until fully migrated.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status