Skip to content

Clickable component#4018

Open
m-bert wants to merge 54 commits intomainfrom
@mbert/clickable
Open

Clickable component#4018
m-bert wants to merge 54 commits intomainfrom
@mbert/clickable

Conversation

@m-bert
Copy link
Contributor

@m-bert m-bert commented Mar 9, 2026

Description

This PR introduces new Clickable component, which is meant to be a replacement for buttons.

Note

Docs for Clickable will be added in #4022, as I don't want to release them right away after merging this PR.

borderless

For now, borderless doesn't work. I've tested clickable with some changes that allow borderless ripple to be visible, however we don't want to introduce them here because it would break other things. Also it should be generl fix, not in the PR with new component.

Stress test

Render list with 2000 buttons 25 times (50ms delay between renders), drop 3 best and 3 worst results. Then calculate average.

Stress test example is available in this PR.

Android

$t_{Button}$ $t_{Clickable}$ $\Delta{t}$
BaseButton 1196,18 1292,3 96,12
RectButton 1672,6 1275,68 -396,92
BorderlessButton 1451,34 1290,74 -160,6

iOS

$t_{Button}$ $t_{Clickable}$ $\Delta{t}$
BaseButton 1101,37 1154,6 53,23
RectButton 1528,07 1160,07 -368
BorderlessButton 1330,24 1172,69 -157,55

Web

$t_{Button}$ $t_{Clickable}$ $\Delta{t}$
BaseButton 64,18 95,57 31,39
RectButton 104,58 97,95 -6,63
BorderlessButton 81,11 98,64 17,53

Test plan

New examples

Copilot AI review requested due to automatic review settings March 9, 2026 16:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new Clickable component in the react-native-gesture-handler library, intended to serve as a unified replacement for BaseButton, RectButton, and BorderlessButton. The component supports configurable visual feedback (underlay or whole-component opacity changes) and native Android ripple effects. Supporting changes include extracting visual style properties (background color, border radius, etc.) through to the native button in GestureHandlerButton, type improvements to RawButton and BorderlessButton, and a new example screen.

Changes:

  • New Clickable component with configurable animation feedback (feedbackTarget, feedbackType, activeOpacity, underlayColor) and long-press support
  • GestureHandlerButton now passes visual style properties (backgroundColor, borderRadius, border styles) to the native ButtonComponent, and RawButton gets proper type generics
  • Example screen demonstrating Clickable in various configurations (base, rect-like, borderless-like, custom)

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/v3/components/Clickable.tsx New Clickable component with animation, long-press, and ripple support
src/v3/components/index.ts Exports Clickable and ClickableProps (from incorrect path)
src/v3/index.ts Re-exports Clickable from components
src/components/GestureHandlerButton.tsx Extracts visual properties (backgroundColor, border*) into buttonStyle passed to native button
src/v3/components/GestureButtons.tsx Adds type generics to RawButton, extracts ref in BorderlessButton
src/v3/types/NativeWrapperType.ts Adds `
apps/common-app/src/new_api/components/clickable/index.tsx Example screen showcasing Clickable configurations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@m-bert m-bert marked this pull request as ready for review March 13, 2026 11:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


export const RawButton = createNativeWrapper(GestureHandlerButton, {
export const RawButton = createNativeWrapper<
ReturnType<typeof GestureHandlerButton>,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to change this to unknown (cc @j-piasecki).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't, but the comment itself is valid. Why ReturnType<typeof GestureHandlerButton> instead of the actual ref type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it might be confusing, but TRef is what is actually wrapped with ref, not ref itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but ReturnType of GestureHandlerButton will be something like ReactNode, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's wrong with React.ComponentRef<typeof GestureHandlerButton>?

Copy link
Contributor Author

@m-bert m-bert Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it redundant? Currently we have:

ref?: React.Ref<T> | undefined;

where T is TRef. So with ComponentRef we have:

ref?: React.Ref<React.ComponentRef<typeof GestureHandlerButton>> | undefined;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the correct type though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OKay, now I looked at internals and it makes sense (at least with given jsdoc)

Comment on lines +125 to +130
onPressOut?.(e);

if (longPressTimeout.current !== undefined) {
clearTimeout(longPressTimeout.current);
longPressTimeout.current = undefined;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have mixed feelings. I don't like adding more hooks to track onPressIn. Maybe we could call onPressOut after checking e.pointerInside. WDYT @j-piasecki?

Copy link
Member

@j-piasecki j-piasecki Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't pointerInside change triggering onUpdate? If so, then yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Android onUpdate is triggered on each move. On iOS it seems that something is blocking it from being sent as in Android (I'm getting it kinda randomly while panning).

Do we wait with Clickable until we resolve this, or do we merge (and possibly ship) this component while looking for a solution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can wait, given that the PR is already up

KINDA_BLUE: '#5f97c8',
ANDROID: '#34a853',
WEB: '#1067c4',
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the common colours, if needed add new colours there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


<View style={styles.section}>
<Text style={styles.sectionHeader}>Custom animations</Text>
<Text>Animated overlay.</Text>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no overlay

/>
</View>

<Text>Animated component.</Text>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it refer to?

// Visual properties
...restStyle
} = flattenedStyle;
} = flattenedStyle ?? {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's possible for flattenedStyle to be nullish, can it be moved higher? I.e.

const flattenedStyle = useMemo(() => StyleSheet.flatten(style) ?? {}, [style]);

Comment on lines +125 to +130
onPressOut?.(e);

if (longPressTimeout.current !== undefined) {
clearTimeout(longPressTimeout.current);
longPressTimeout.current = undefined;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can wait, given that the PR is already up

Comment on lines +59 to +61
if (!isAndroid || !e.pointerInside) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed if we unify the native handler events, right?

Comment on lines +73 to +76
if (!isAndroid && e.pointerInside) {
onPressIn?.(e);
startLongPressTimer();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

Comment on lines +39 to +42
const longPressDetected = useRef(false);
const longPressTimeout = useRef<ReturnType<typeof setTimeout> | undefined>(
undefined
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a visible performance impact of creating two refs instead of a single { detected: boolean, timeout: Timeout } one?

onLongPress,
onPress,
onPressIn,
onPressOut,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onPressOut is being called twice on iOS when dragging outside of the button (+ the 100px retention 🤦)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants