-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[TS migration] Migrate 'Button' component to TypeScript #31102
Conversation
src/components/Button/index.tsx
Outdated
|
||
/** Any additional styles to pass to the left icon container. */ | ||
// eslint-disable-next-line react/forbid-prop-types | ||
iconStyles: PropTypes.arrayOf(PropTypes.object), | ||
iconStyles?: Array<StyleProp<ViewStyle>>; |
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.
Just StyleProp<ViewStyle>
should be enough, then in the code replace ...iconStyles
with just iconStyles
, it should work the same way as before
iconStyles?: Array<StyleProp<ViewStyle>>; | |
iconStyles?: StyleProp<ViewStyle>; |
src/components/Button/index.tsx
Outdated
|
||
/** Any additional styles to pass to the right icon container. */ | ||
// eslint-disable-next-line react/forbid-prop-types | ||
iconRightStyles: PropTypes.arrayOf(PropTypes.object), | ||
iconRightStyles?: Array<StyleProp<ViewStyle>>; |
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.
Same
iconRightStyles?: Array<StyleProp<ViewStyle>>; | |
iconRightStyles?: StyleProp<ViewStyle>; |
src/components/Button/index.tsx
Outdated
|
||
/** A function that is called when the button is clicked on */ | ||
onPress: PropTypes.func, | ||
onPress?: (e?: GestureResponderEvent | KeyboardEvent) => void; |
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.
onPress?: (e?: GestureResponderEvent | KeyboardEvent) => void; | |
onPress?: (event?: GestureResponderEvent | KeyboardEvent) => void; |
src/components/Button/index.tsx
Outdated
|
||
/** A function that is called when the button is long pressed */ | ||
onLongPress: PropTypes.func, | ||
onLongPress?: (e?: GestureResponderEvent) => void; |
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.
onLongPress?: (e?: GestureResponderEvent) => void; | |
onLongPress?: (event?: GestureResponderEvent) => void; |
src/components/Button/index.tsx
Outdated
|
||
/** 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Why not StyleProp<ViewStyle>
?
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.
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.
It looks a little bit strange. How do you think can we use just use style
instead of ...StyleUtils.parseStyleAsArray(style)
?
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 agree, let's just use style
, parseStyleAsArray
isn't needed
@@ -318,6 +279,7 @@ 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
icon || shouldShowRightIcon ? styles.alignItemsStretch : undefined
i gues for if statments we should use normal OR operator
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.
You can try !!icon || shouldShowRightIcon....
, eslint should pass it so you can remove eslint disable
src/components/Button/index.tsx
Outdated
|
||
ButtonWithRef.displayName = 'ButtonWithRef'; | ||
|
||
export default withNavigationFallback(ButtonWithRef); | ||
export default withNavigationFallback(React.forwardRef(ButtonWithRef)); |
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.
If you add ref as a second param you can do it this way
export default withNavigationFallback(React.forwardRef(ButtonWithRef)); | |
export default withNavigationFallback(React.forwardRef(Button)); |
src/components/Button/index.tsx
Outdated
@@ -282,7 +242,8 @@ function Button({ | |||
ref={forwardedRef} | |||
onPress={(event) => { | |||
if (event && event.type === 'click') { |
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.
if (event && event.type === 'click') { | |
if (event?.type === 'click') { |
@@ -282,7 +242,8 @@ function Button({ | |||
ref={forwardedRef} | |||
onPress={(event) => { | |||
if (event && event.type === 'click') { | |||
event.currentTarget.blur(); | |||
const currentTarget = event?.currentTarget as HTMLElement; |
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.
Why we need assertion here?
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.
without assertion ts thinks that currentTarget
is type of number | EventTarget
on which blur
function is not existing but we can assert that its HTMLElement
because click
event type is only on web or desktop
*/ | ||
|
||
const validateSubmitShortcut: ValidateSubmitShortcut = (isFocused, isDisabled, isLoading, event) => { | ||
const eventTarget = event?.target as HTMLElement; |
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.
Same question about assertion :)
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.
similar situation as here
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove withNavigationFallback
now? And use navigation hook instead @VickyStash
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.
@blazejkustra It looks like it should be possible, @kubabutkiewicz is going to give it a try
src/components/Button/index.tsx
Outdated
|
||
/** 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's just use style
, parseStyleAsArray
isn't needed
src/components/Button/index.tsx
Outdated
@@ -16,195 +15,154 @@ import themeColors from '@styles/themes/default'; | |||
import CONST from '@src/CONST'; | |||
import validateSubmitShortcut from './validateSubmitShortcut'; | |||
|
|||
const propTypes = { | |||
type ButtonProps = { |
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:
import ChildrenProps from '@src/types/utils/ChildrenProps';
type ButtonWithText = {
/** The text for the button label */
text: string;
/** Boolean whether to display the right icon */
shouldShowRightIcon?: boolean;
/** The icon asset to display to the left of the text */
icon?: React.FC<SvgProps> | null;
};
type ButtonProps = (ButtonWithText | ChildrenProps) & { ... }
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:
function Button(
{
allowBubble = false,
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,
shouldRemoveRightBorderRadius = false,
shouldRemoveLeftBorderRadius = false,
shouldEnableHapticFeedback = false,
id = '',
accessibilityLabel = '',
...rest
}: ButtonProps,
ref: ForwardedRef<View>,
) {
const renderContent = () => {
if ('children' in rest) {
return rest.children;
}
const {text = '', icon = null, shouldShowRightIcon = false} = rest;
@allroundexperts Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
…s-migration/Button/component
…s-migration/Button/component
@allroundexperts kind bump 😄 |
@@ -238,20 +193,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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
@allroundexperts
Actually spread operator is not needed here
we can pass array or object without spreading and styles will still work
if (icon || shouldShowRightIcon) { | ||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Why is ...
being removed?
@@ -262,7 +218,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 comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
icon || shouldShowRightIcon ? styles.alignItemsStretch : undefined, | ||
...innerStyles, | ||
// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing | ||
'text' in rest && (rest?.icon || rest?.shouldShowRightIcon) ? styles.alignItemsStretch : undefined, |
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.
Not sure what you're trying to do here. Why 'text' in rest
is being added? Why is it not using ?
?
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 is just for type narrowing , there are two ways of using Button component one with children like
<Button>Hello</Button
or with text props like
<Button text="Hello" />
and those two props icon
and shouldShowRightIcon
can be used only when Button
is used with text
prop. 'text' in rest
will assure that Button
is used with text
prop
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.
about the ?
its needed only when we are using object dot notation so if rest
is undefined it will be safe to use properties of the object. when using 'text' in rest
and rest is undefined
it will just return false
src/components/Button/index.tsx
Outdated
ButtonWithRef.displayName = 'ButtonWithRef'; | ||
|
||
export default withNavigationFallback(ButtonWithRef); | ||
export default React.forwardRef(Button); |
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.
Why is the withNavigationFallback
being removed?
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.
instead of the withNavigationFallback
we can use useIsFocused
which we are doing , so withNavigationFallback
is not needed anymore
…s-migration/Button/component
…s-migration/Button/component
@allroundexperts kind bump 😄 |
I have triggered a build for this PR, @allroundexperts i know you have lots on your plate, this is one of 2 most important/ most blocking TS migration PRs now, could we reassign to someone who can take this over the finish line as soon as possible? the transition bug you are working on for instance is higher priority though |
@kubabutkiewicz can you please check the reassure performance tests failing? this should not be happening on the migration PR |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
…s-migration/Button/component
…s-migration/Button/component
@mountiny @allroundexperts Alternatively @parasharrajat could take over as he has a lot of context |
Sorry, I missed the previous notifications for this. I have noted this down and will finish this today. |
…s-migration/Button/component
I can review this immediately, if needed. |
Thank you @parasharrajat. But I've started already. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2023-11-21.at.11.09.26.PM.movAndroid: mWeb ChromeScreen.Recording.2023-11-21.at.11.08.46.PM.moviOS: NativeScreen.Recording.2023-11-21.at.11.15.04.PM.moviOS: mWeb SafariScreen.Recording.2023-11-21.at.11.13.00.PM.movMacOS: Chrome / SafariScreen.Recording.2023-11-21.at.11.00.40.PM.movMacOS: DesktopScreen.Recording.2023-11-21.at.11.04.42.PM.mov |
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.
mweb
screenshots are missing but since this is urgent, I'm approving this.
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25098 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
…s-migration/Button/component
@srikarparsi got the review on this one @allroundexperts thanks for the review |
…s-migration/Button/component
…s-migration/Button/component
…s-migration/Button/component
@srikarparsi kindly bump 🙏 |
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.
lgtm
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/srikarparsi in version: 1.4.4-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.4-3 🚀
|
Details
Fixed Issues
$ #25098
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.mp4
Android: mWeb Chrome
iOS: Native
ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4