Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Button.js to function component #30349

315 changes: 166 additions & 149 deletions src/components/Button/index.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
import {useIsFocused} from '@react-navigation/native';
import PropTypes from 'prop-types';
import React, {Component} from 'react';
import React, {useCallback} from 'react';
import {ActivityIndicator, View} from 'react-native';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import PressableWithFeedback from '@components/Pressable/PressableWithFeedback';
import refPropTypes from '@components/refPropTypes';
import Text from '@components/Text';
import withNavigationFallback from '@components/withNavigationFallback';
import withNavigationFocus from '@components/withNavigationFocus';
import compose from '@libs/compose';
import useKeyboardShortcut from '@hooks/useKeyboardShortcut';
import HapticFeedback from '@libs/HapticFeedback';
import KeyboardShortcut from '@libs/KeyboardShortcut';
import styles from '@styles/styles';
import * as StyleUtils from '@styles/StyleUtils';
import themeColors from '@styles/themes/default';
Expand Down Expand Up @@ -112,9 +111,6 @@ const propTypes = {
/** Should enable the haptic feedback? */
shouldEnableHapticFeedback: PropTypes.bool,

/** Whether Button is on active screen */
isFocused: PropTypes.bool.isRequired,

/** Id to use for this button */
nativeID: PropTypes.string,

Expand Down Expand Up @@ -161,94 +157,117 @@ const defaultProps = {
forwardedRef: undefined,
};

class Button extends Component {
constructor(props) {
super(props);

this.renderContent = this.renderContent.bind(this);
}

componentDidMount() {
if (!this.props.pressOnEnter) {
return;
}

const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;

// Setup and attach keypress handler for pressing the button with Enter key
this.unsubscribe = KeyboardShortcut.subscribe(
shortcutConfig.shortcutKey,
(e) => {
if (!validateSubmitShortcut(this.props.isFocused, this.props.isDisabled, this.props.isLoading, e)) {
return;
}
this.props.onPress();
},
shortcutConfig.descriptionKey,
shortcutConfig.modifiers,
true,
this.props.allowBubble,
this.props.enterKeyEventListenerPriority,
false,
);
}

componentWillUnmount() {
// Cleanup event listeners
if (!this.unsubscribe) {
return;
}
this.unsubscribe();
}

renderContent() {
if (this.props.children) {
return this.props.children;
function Button({
allowBubble,
text,
shouldShowRightIcon,

icon,
iconRight,
iconFill,
iconStyles,
iconRightStyles,

small,
large,
medium,

isLoading,
isDisabled,

onPress,
onLongPress,
onPressIn,
onPressOut,
onMouseDown,

pressOnEnter,
enterKeyEventListenerPriority,

style,
innerStyles,
textStyles,

shouldUseDefaultHover,
success,
danger,
children,

shouldRemoveRightBorderRadius,
shouldRemoveLeftBorderRadius,
shouldEnableHapticFeedback,

fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved
nativeID,
accessibilityLabel,
forwardedRef,
}) {
const isFocused = useIsFocused();

const keyboardShortcutCallback = useCallback(
(event) => {
Copy link
Member

Choose a reason for hiding this comment

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

We should use useCallback here.

Copy link
Member

Choose a reason for hiding this comment

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

@kubabutkiewicz Thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree @parasharrajat , will adjust that

if (!validateSubmitShortcut(isFocused, isDisabled, isLoading, event)) {
return;
}
onPress();
},
[isDisabled, isFocused, isLoading, onPress],
);

useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ENTER, keyboardShortcutCallback, {
isActive: pressOnEnter,
shouldBubble: allowBubble,
priority: enterKeyEventListenerPriority,
shouldPreventDefault: false,
});
Comment on lines +206 to +221
Copy link
Contributor

Choose a reason for hiding this comment

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

This caused a regression #33764. Previously we used to subscribe only once (componentDidMount). Now the useKeyboardShortcut hook re-runs based on dependencies. One of the dependencies is the keyboardShortcutCallback. This one also relies on other dependencies including isFocused. The isFocused value changes when the screen is unfocused/focused which causes the keyboard subscription to run again. When the keyboard subscription runs again it gets the highest priority and messes with other pages.

Explanation
Before:

  1. Mount page 1 -> Add keyboard listener for page 1 -> Mount page 2 -> Add keyboard listener for page 2
  2. Pressing ENTER will trigger on page 2

After:

  1. Mount page 1 -> Add keyboard listener for page 1 -> Mount page 2 -> Add keyboard listener for page 2 -> Page 1 is now unmouted -> Add keyboard listener for page 1 (since the useEffect triggers on page unfocus)
  2. Pressing ENTER will trigger on page 1


const renderContent = () => {
fabioh8010 marked this conversation as resolved.
Show resolved Hide resolved
if (children) {
return children;
}

const textComponent = (
<Text
numberOfLines={1}
selectable={false}
style={[
this.props.isLoading && styles.opacity0,
isLoading && styles.opacity0,
styles.pointerEventsNone,
styles.buttonText,
this.props.small && styles.buttonSmallText,
this.props.medium && styles.buttonMediumText,
this.props.large && styles.buttonLargeText,
this.props.success && styles.buttonSuccessText,
this.props.danger && styles.buttonDangerText,
this.props.icon && styles.textAlignLeft,
...this.props.textStyles,
small && styles.buttonSmallText,
medium && styles.buttonMediumText,
large && styles.buttonLargeText,
success && styles.buttonSuccessText,
danger && styles.buttonDangerText,
icon && styles.textAlignLeft,
...textStyles,
]}
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}}
>
{this.props.text}
{text}
</Text>
);

if (this.props.icon || this.props.shouldShowRightIcon) {
if (icon || shouldShowRightIcon) {
return (
<View style={[styles.justifyContentBetween, styles.flexRow]}>
<View style={[styles.alignItemsCenter, styles.flexRow, styles.flexShrink1]}>
{this.props.icon && (
<View style={[styles.mr1, ...this.props.iconStyles]}>
{icon && (
<View style={[styles.mr1, ...iconStyles]}>
<Icon
src={this.props.icon}
fill={this.props.iconFill}
small={this.props.small}
src={icon}
fill={iconFill}
small={small}
/>
</View>
)}
{textComponent}
</View>
{this.props.shouldShowRightIcon && (
<View style={[styles.justifyContentCenter, styles.ml1, ...this.props.iconRightStyles]}>
{shouldShowRightIcon && (
<View style={[styles.justifyContentCenter, styles.ml1, ...iconRightStyles]}>
<Icon
src={this.props.iconRight}
fill={this.props.iconFill}
small={this.props.small}
src={iconRight}
fill={iconFill}
small={small}
/>
</View>
)}
Expand All @@ -257,87 +276,85 @@ class Button extends Component {
}

return textComponent;
}

render() {
return (
<PressableWithFeedback
ref={this.props.forwardedRef}
onPress={(e) => {
if (e && e.type === 'click') {
e.currentTarget.blur();
}

if (this.props.shouldEnableHapticFeedback) {
HapticFeedback.press();
}
return this.props.onPress(e);
}}
onLongPress={(e) => {
if (this.props.shouldEnableHapticFeedback) {
HapticFeedback.longPress();
}
this.props.onLongPress(e);
}}
onPressIn={this.props.onPressIn}
onPressOut={this.props.onPressOut}
onMouseDown={this.props.onMouseDown}
disabled={this.props.isLoading || this.props.isDisabled}
wrapperStyle={[
this.props.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...StyleUtils.parseStyleAsArray(this.props.style),
]}
style={[
styles.button,
this.props.small ? styles.buttonSmall : undefined,
this.props.medium ? styles.buttonMedium : undefined,
this.props.large ? styles.buttonLarge : undefined,
this.props.success ? styles.buttonSuccess : undefined,
this.props.danger ? styles.buttonDanger : undefined,
this.props.isDisabled && (this.props.success || this.props.danger) ? styles.buttonOpacityDisabled : undefined,
this.props.isDisabled && !this.props.danger && !this.props.success ? styles.buttonDisabled : undefined,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
this.props.icon || this.props.shouldShowRightIcon ? styles.alignItemsStretch : undefined,
...this.props.innerStyles,
]}
hoverStyle={[
this.props.shouldUseDefaultHover && !this.props.isDisabled ? styles.buttonDefaultHovered : undefined,
this.props.success && !this.props.isDisabled ? styles.buttonSuccessHovered : undefined,
this.props.danger && !this.props.isDisabled ? styles.buttonDangerHovered : undefined,
]}
nativeID={this.props.nativeID}
accessibilityLabel={this.props.accessibilityLabel}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
hoverDimmingValue={1}
>
{this.renderContent()}
{this.props.isLoading && (
<ActivityIndicator
color={this.props.success || this.props.danger ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</PressableWithFeedback>
);
}
};

return (
<PressableWithFeedback
ref={forwardedRef}
onPress={(event) => {
if (event && event.type === 'click') {
event.currentTarget.blur();
}

if (shouldEnableHapticFeedback) {
HapticFeedback.press();
}
return onPress(event);
}}
onLongPress={(event) => {
if (shouldEnableHapticFeedback) {
HapticFeedback.longPress();
}
onLongPress(event);
}}
onPressIn={onPressIn}
onPressOut={onPressOut}
onMouseDown={onMouseDown}
disabled={isLoading || isDisabled}
wrapperStyle={[
isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
...StyleUtils.parseStyleAsArray(style),
]}
style={[
styles.button,
small ? styles.buttonSmall : undefined,
medium ? styles.buttonMedium : undefined,
large ? styles.buttonLarge : undefined,
success ? styles.buttonSuccess : undefined,
danger ? styles.buttonDanger : undefined,
isDisabled && (success || danger) ? styles.buttonOpacityDisabled : undefined,
isDisabled && !danger && !success ? styles.buttonDisabled : undefined,
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
icon || shouldShowRightIcon ? styles.alignItemsStretch : undefined,
...innerStyles,
]}
hoverStyle={[
shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined,
success && !isDisabled ? styles.buttonSuccessHovered : undefined,
danger && !isDisabled ? styles.buttonDangerHovered : undefined,
]}
nativeID={nativeID}
accessibilityLabel={accessibilityLabel}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
hoverDimmingValue={1}
>
{renderContent()}
{isLoading && (
<ActivityIndicator
color={success || danger ? themeColors.textLight : themeColors.text}
style={[styles.pAbsolute, styles.l0, styles.r0]}
/>
)}
</PressableWithFeedback>
);
}

Button.propTypes = propTypes;
Button.defaultProps = defaultProps;
Button.displayName = 'Button';

const ButtonWithRef = React.forwardRef((props, ref) => (
<Button
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
forwardedRef={ref}
/>
));

ButtonWithRef.displayName = 'ButtonWithRef';

export default compose(
withNavigationFallback,
withNavigationFocus,
)(
React.forwardRef((props, ref) => (
<Button
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
forwardedRef={ref}
/>
)),
);
export default withNavigationFallback(ButtonWithRef);
Loading