-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[TS migration] Migrate 'Button' component to TypeScript #31102
Changes from 8 commits
db66206
ec3f466
766c7d5
4315fee
aa16641
060fc5b
a353175
defdcac
5c707c1
0f86446
d2f719d
2cf21f5
f358f92
1d9e673
ae4e159
3774805
a388735
9154332
ddb7674
9e359a2
d4bf0ee
46825fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,11 +1,10 @@ | ||||||||
import {useIsFocused} from '@react-navigation/native'; | ||||||||
import PropTypes from 'prop-types'; | ||||||||
import React, {useCallback} from 'react'; | ||||||||
import {ActivityIndicator, View} from 'react-native'; | ||||||||
import React, {ForwardedRef, useCallback} from 'react'; | ||||||||
import {ActivityIndicator, GestureResponderEvent, StyleProp, TextStyle, View, ViewStyle} from 'react-native'; | ||||||||
import {SvgProps} from 'react-native-svg'; | ||||||||
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 useKeyboardShortcut from '@hooks/useKeyboardShortcut'; | ||||||||
|
@@ -16,195 +15,154 @@ import themeColors from '@styles/themes/default'; | |||||||
import CONST from '@src/CONST'; | ||||||||
import validateSubmitShortcut from './validateSubmitShortcut'; | ||||||||
|
||||||||
const propTypes = { | ||||||||
type ButtonProps = { | ||||||||
/** Should the press event bubble across multiple instances when Enter key triggers it. */ | ||||||||
allowBubble: PropTypes.bool, | ||||||||
allowBubble?: boolean; | ||||||||
|
||||||||
/** The text for the button label */ | ||||||||
text: PropTypes.string, | ||||||||
text?: string; | ||||||||
|
||||||||
/** Boolean whether to display the right icon */ | ||||||||
shouldShowRightIcon: PropTypes.bool, | ||||||||
shouldShowRightIcon?: boolean; | ||||||||
|
||||||||
/** The icon asset to display to the left of the text */ | ||||||||
icon: PropTypes.func, | ||||||||
icon?: React.FC<SvgProps> | null; | ||||||||
|
||||||||
/** The icon asset to display to the right of the text */ | ||||||||
iconRight: PropTypes.func, | ||||||||
iconRight?: React.FC<SvgProps>; | ||||||||
|
||||||||
/** The fill color to pass into the icon. */ | ||||||||
iconFill: PropTypes.string, | ||||||||
iconFill?: string; | ||||||||
|
||||||||
/** Any additional styles to pass to the left icon container. */ | ||||||||
// eslint-disable-next-line react/forbid-prop-types | ||||||||
iconStyles: PropTypes.arrayOf(PropTypes.object), | ||||||||
iconStyles?: StyleProp<ViewStyle>; | ||||||||
|
||||||||
/** Any additional styles to pass to the right icon container. */ | ||||||||
// eslint-disable-next-line react/forbid-prop-types | ||||||||
iconRightStyles: PropTypes.arrayOf(PropTypes.object), | ||||||||
iconRightStyles?: StyleProp<ViewStyle>; | ||||||||
|
||||||||
/** Small sized button */ | ||||||||
small: PropTypes.bool, | ||||||||
small?: boolean; | ||||||||
|
||||||||
/** Large sized button */ | ||||||||
large: PropTypes.bool, | ||||||||
large?: boolean; | ||||||||
|
||||||||
/** medium sized button */ | ||||||||
medium: PropTypes.bool, | ||||||||
/** Medium sized button */ | ||||||||
medium?: boolean; | ||||||||
|
||||||||
/** Indicates whether the button should be disabled and in the loading state */ | ||||||||
isLoading: PropTypes.bool, | ||||||||
isLoading?: boolean; | ||||||||
|
||||||||
/** Indicates whether the button should be disabled */ | ||||||||
isDisabled: PropTypes.bool, | ||||||||
isDisabled?: boolean; | ||||||||
|
||||||||
/** A function that is called when the button is clicked on */ | ||||||||
onPress: PropTypes.func, | ||||||||
onPress?: (event?: GestureResponderEvent | KeyboardEvent) => void; | ||||||||
|
||||||||
/** A function that is called when the button is long pressed */ | ||||||||
onLongPress: PropTypes.func, | ||||||||
onLongPress?: (event?: GestureResponderEvent) => void; | ||||||||
|
||||||||
/** A function that is called when the button is pressed */ | ||||||||
onPressIn: PropTypes.func, | ||||||||
onPressIn?: () => void; | ||||||||
|
||||||||
/** A function that is called when the button is released */ | ||||||||
onPressOut: PropTypes.func, | ||||||||
onPressOut?: () => void; | ||||||||
|
||||||||
/** Callback that is called when mousedown is triggered. */ | ||||||||
onMouseDown: PropTypes.func, | ||||||||
onMouseDown?: () => void; | ||||||||
|
||||||||
/** Call the onPress function when Enter key is pressed */ | ||||||||
pressOnEnter: PropTypes.bool, | ||||||||
pressOnEnter?: boolean; | ||||||||
|
||||||||
/** The priority to assign the enter key event listener. 0 is the highest priority. */ | ||||||||
enterKeyEventListenerPriority: PropTypes.number, | ||||||||
enterKeyEventListenerPriority?: number; | ||||||||
|
||||||||
/** Additional styles to add after local styles. Applied to Pressable portion of button */ | ||||||||
style: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]), | ||||||||
style?: ViewStyle | ViewStyle[]; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks a little bit strange. How do you think can we use just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, let's just use |
||||||||
|
||||||||
/** Additional button styles. Specific to the OpacityView of button */ | ||||||||
// eslint-disable-next-line react/forbid-prop-types | ||||||||
innerStyles: PropTypes.arrayOf(PropTypes.object), | ||||||||
/** Additional button styles. Specific to the OpacityView of the button */ | ||||||||
innerStyles?: StyleProp<ViewStyle>; | ||||||||
|
||||||||
/** Additional text styles */ | ||||||||
// eslint-disable-next-line react/forbid-prop-types | ||||||||
textStyles: PropTypes.arrayOf(PropTypes.object), | ||||||||
textStyles?: StyleProp<TextStyle>; | ||||||||
|
||||||||
/** Whether we should use the default hover style */ | ||||||||
shouldUseDefaultHover: PropTypes.bool, | ||||||||
shouldUseDefaultHover?: boolean; | ||||||||
|
||||||||
/** Whether we should use the success theme color */ | ||||||||
success: PropTypes.bool, | ||||||||
success?: boolean; | ||||||||
|
||||||||
/** Whether we should use the danger theme color */ | ||||||||
danger: PropTypes.bool, | ||||||||
danger?: boolean; | ||||||||
|
||||||||
/** Children to replace all inner contents of button */ | ||||||||
children: PropTypes.node, | ||||||||
/** Children to replace all inner contents of the button */ | ||||||||
children?: React.ReactNode; | ||||||||
|
||||||||
/** Should we remove the right border radius top + bottom? */ | ||||||||
shouldRemoveRightBorderRadius: PropTypes.bool, | ||||||||
shouldRemoveRightBorderRadius?: boolean; | ||||||||
|
||||||||
/** Should we remove the left border radius top + bottom? */ | ||||||||
shouldRemoveLeftBorderRadius: PropTypes.bool, | ||||||||
shouldRemoveLeftBorderRadius?: boolean; | ||||||||
|
||||||||
/** Should enable the haptic feedback? */ | ||||||||
shouldEnableHapticFeedback: PropTypes.bool, | ||||||||
shouldEnableHapticFeedback?: boolean; | ||||||||
|
||||||||
/** Id to use for this button */ | ||||||||
id: PropTypes.string, | ||||||||
id?: string; | ||||||||
|
||||||||
/** Accessibility label for the component */ | ||||||||
accessibilityLabel: PropTypes.string, | ||||||||
|
||||||||
/** A ref to forward the button */ | ||||||||
forwardedRef: refPropTypes, | ||||||||
}; | ||||||||
|
||||||||
const defaultProps = { | ||||||||
allowBubble: false, | ||||||||
text: '', | ||||||||
shouldShowRightIcon: false, | ||||||||
icon: null, | ||||||||
iconRight: Expensicons.ArrowRight, | ||||||||
iconFill: themeColors.textLight, | ||||||||
iconStyles: [], | ||||||||
iconRightStyles: [], | ||||||||
isLoading: false, | ||||||||
isDisabled: false, | ||||||||
small: false, | ||||||||
large: false, | ||||||||
medium: false, | ||||||||
onPress: () => {}, | ||||||||
onLongPress: () => {}, | ||||||||
onPressIn: () => {}, | ||||||||
onPressOut: () => {}, | ||||||||
onMouseDown: undefined, | ||||||||
pressOnEnter: false, | ||||||||
enterKeyEventListenerPriority: 0, | ||||||||
style: [], | ||||||||
innerStyles: [], | ||||||||
textStyles: [], | ||||||||
shouldUseDefaultHover: true, | ||||||||
success: false, | ||||||||
danger: false, | ||||||||
children: null, | ||||||||
shouldRemoveRightBorderRadius: false, | ||||||||
shouldRemoveLeftBorderRadius: false, | ||||||||
shouldEnableHapticFeedback: false, | ||||||||
id: '', | ||||||||
accessibilityLabel: '', | ||||||||
forwardedRef: undefined, | ||||||||
accessibilityLabel?: string; | ||||||||
}; | ||||||||
|
||||||||
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, | ||||||||
|
||||||||
id, | ||||||||
accessibilityLabel, | ||||||||
forwardedRef, | ||||||||
}) { | ||||||||
function Button( | ||||||||
{ | ||||||||
allowBubble = false, | ||||||||
text = '', | ||||||||
shouldShowRightIcon = false, | ||||||||
|
||||||||
icon = null, | ||||||||
iconRight = Expensicons.ArrowRight, | ||||||||
iconFill = themeColors.textLight, | ||||||||
iconStyles = [], | ||||||||
iconRightStyles = [], | ||||||||
|
||||||||
small = false, | ||||||||
large = false, | ||||||||
medium = false, | ||||||||
|
||||||||
isLoading = false, | ||||||||
isDisabled = false, | ||||||||
|
||||||||
onPress = () => {}, | ||||||||
onLongPress = () => {}, | ||||||||
onPressIn = () => {}, | ||||||||
onPressOut = () => {}, | ||||||||
onMouseDown = undefined, | ||||||||
|
||||||||
pressOnEnter = false, | ||||||||
enterKeyEventListenerPriority = 0, | ||||||||
|
||||||||
style = [], | ||||||||
innerStyles = [], | ||||||||
textStyles = [], | ||||||||
|
||||||||
shouldUseDefaultHover = true, | ||||||||
success = false, | ||||||||
danger = false, | ||||||||
children = null, | ||||||||
|
||||||||
shouldRemoveRightBorderRadius = false, | ||||||||
shouldRemoveLeftBorderRadius = false, | ||||||||
shouldEnableHapticFeedback = false, | ||||||||
|
||||||||
id = '', | ||||||||
accessibilityLabel = '', | ||||||||
}: ButtonProps, | ||||||||
ref: ForwardedRef<View>, | ||||||||
) { | ||||||||
const isFocused = useIsFocused(); | ||||||||
|
||||||||
const keyboardShortcutCallback = useCallback( | ||||||||
(event) => { | ||||||||
(event?: GestureResponderEvent | KeyboardEvent) => { | ||||||||
if (!validateSubmitShortcut(isFocused, isDisabled, isLoading, event)) { | ||||||||
return; | ||||||||
} | ||||||||
|
@@ -238,20 +196,21 @@ function Button({ | |||||||
success && styles.buttonSuccessText, | ||||||||
danger && styles.buttonDangerText, | ||||||||
icon && styles.textAlignLeft, | ||||||||
...textStyles, | ||||||||
textStyles, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @allroundexperts ![]() |
||||||||
]} | ||||||||
dataSet={{[CONST.SELECTION_SCRAPER_HIDDEN_ELEMENT]: true}} | ||||||||
> | ||||||||
{text} | ||||||||
</Text> | ||||||||
); | ||||||||
|
||||||||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||||||||
if (icon || shouldShowRightIcon) { | ||||||||
Comment on lines
+208
to
209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
return ( | ||||||||
<View style={[styles.justifyContentBetween, styles.flexRow]}> | ||||||||
<View style={[styles.alignItemsCenter, styles.flexRow, styles.flexShrink1]}> | ||||||||
{icon && ( | ||||||||
<View style={[styles.mr1, ...iconStyles]}> | ||||||||
<View style={[styles.mr1, iconStyles]}> | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same. Why is |
||||||||
<Icon | ||||||||
src={icon} | ||||||||
fill={iconFill} | ||||||||
|
@@ -262,7 +221,7 @@ function Button({ | |||||||
{textComponent} | ||||||||
</View> | ||||||||
{shouldShowRightIcon && ( | ||||||||
<View style={[styles.justifyContentCenter, styles.ml1, ...iconRightStyles]}> | ||||||||
<View style={[styles.justifyContentCenter, styles.ml1, iconRightStyles]}> | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||||||||
<Icon | ||||||||
src={iconRight} | ||||||||
fill={iconFill} | ||||||||
|
@@ -279,10 +238,11 @@ function Button({ | |||||||
|
||||||||
return ( | ||||||||
<PressableWithFeedback | ||||||||
ref={forwardedRef} | ||||||||
ref={ref} | ||||||||
onPress={(event) => { | ||||||||
if (event && event.type === 'click') { | ||||||||
event.currentTarget.blur(); | ||||||||
if (event?.type === 'click') { | ||||||||
const currentTarget = event?.currentTarget as HTMLElement; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need assertion here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. without assertion ts thinks that |
||||||||
currentTarget?.blur(); | ||||||||
} | ||||||||
|
||||||||
if (shouldEnableHapticFeedback) { | ||||||||
|
@@ -318,8 +278,9 @@ function Button({ | |||||||
isDisabled && !danger && !success ? styles.buttonDisabled : undefined, | ||||||||
shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined, | ||||||||
shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined, | ||||||||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not nullish coalescing in this place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can try |
||||||||
icon || shouldShowRightIcon ? styles.alignItemsStretch : undefined, | ||||||||
...innerStyles, | ||||||||
innerStyles, | ||||||||
]} | ||||||||
hoverStyle={[ | ||||||||
shouldUseDefaultHover && !isDisabled ? styles.buttonDefaultHovered : undefined, | ||||||||
|
@@ -342,18 +303,6 @@ function Button({ | |||||||
); | ||||||||
} | ||||||||
|
||||||||
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 withNavigationFallback(ButtonWithRef); | ||||||||
export default withNavigationFallback(React.forwardRef(Button)); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blazejkustra It looks like it should be possible, @kubabutkiewicz is going to give it a try |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are two ways of using a Button, either with children or with text and icons:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes destructing a little bit more tricky: