MetaMask / MetaMask/metamask-mobile

[Bug]: ButtonBase accepts both 'disabled' and 'isDisabled' props causing API confusion

Open
#24,179 0 comments 0 reactions 1 assignee Claimed by @georgewrmarshall View on GitHub
INVALID-ISSUE-TEMPLATE team-design-system type-bug
Dominant language
TypeScript
Stars
3k
Forks
1.7k
Avg merge
1d 14h
Merged PRs (30d)
669

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:

1. TypeScript allows passing both `disabled` (inherited from `TouchableOpacityProps`) and `isDisabled` (custom prop)
2. Only `isDisabled` is properly handled by the component logic
3. If someone passes `disabled` instead of `isDisabled`, it may not work as expected due to the explicit `disabled={isDisabled}` mapping on line 45

**Affected Component**: `app/component-library/components/Buttons/Button/foundation/ButtonBase`

**Code References**:
- `ButtonBase.types.ts:12` - Interface extends `TouchableOpacityProps`
- `ButtonBase.tsx:33` - `isDisabled` prop is destructured
- `ButtonBase.tsx:45` - `disabled={isDisabled}` explicitly maps the custom prop
- `ButtonBase.tsx:51` - `{...props}` spreads remaining props, potentially including inherited `disabled`

## 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

1. Open `app/component-library/components/Buttons/Button/foundation/ButtonBase/ButtonBase.types.ts`
2. Note that `ButtonBaseProps extends TouchableOpacityProps` (line 12)
3. See that both `disabled` (from TouchableOpacity) and `isDisabled` (custom) are technically valid props
4. Check `ButtonBase.tsx:45` where only `isDisabled` is used: `disabled={isDisabled}`
5. Attempt to use `` - TypeScript won't complain but it may not work correctly

## Proposed Solution

Explicitly omit the `disabled` prop from `TouchableOpacityProps`:

```typescript
export interface ButtonBaseProps extends Omit {
// ... existing props
isDisabled?: boolean;
}
```

This ensures:
- Only `isDisabled` is 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.