[TS migration] Migrate TextLink to Typescript #30907
Conversation
|
My bad @situchan! This shouldn't yet go to ready for review. This should be reviewed internally first and then cross reviewed by Callstack. I'll mark as ready for review once this is done. |
src/components/TextLink.tsx
Outdated
| const TextLinkWithRef: FC<TextLinkProps> = React.forwardRef<RNText, TextLinkProps>((props, ref) => ( | ||
| <TextLink | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} | ||
| forwardedRef={ref} | ||
| /> | ||
| )); | ||
|
|
||
| TextLinkWithRef.displayName = 'TextLinkWithRef'; |
There was a problem hiding this comment.
You can remove this part
src/components/TextLink.tsx
Outdated
|
|
||
| TextLinkWithRef.displayName = 'TextLinkWithRef'; | ||
|
|
||
| export default TextLinkWithRef; |
There was a problem hiding this comment.
| export default TextLinkWithRef; | |
| export default forwardRef(TextLink); |
src/components/TextLink.tsx
Outdated
| forwardedRef: ForwardedRef<RNText>; | ||
| }; | ||
|
|
||
| function TextLink({href, children, style, onPress, onMouseDown = (event) => event.preventDefault(), forwardedRef, ...props}: TextLinkProps) { |
There was a problem hiding this comment.
According to typescript guidelines forwardRef should be handled a bit differently. Please remove forwardedRef: ForwardedRef<RNText>; prop and move it to the second parameter like this:
| function TextLink({href, children, style, onPress, onMouseDown = (event) => event.preventDefault(), forwardedRef, ...props}: TextLinkProps) { | |
| function TextLink({href, children, style, onPress, onMouseDown = (event) => event.preventDefault(), ...props}: TextLinkProps, ref: ForwardedRef<RNText>) { |
src/components/TextLink.tsx
Outdated
| }; | ||
|
|
||
| const openLinkOnEnterKey: KeyboardEventHandler = (event) => { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
Why event.preventDefault(); was moved from openLink? I advise against logic changes in TS migrations
There was a problem hiding this comment.
I hope there's no logic change in here. I couldn't call openLink(event) from openLinkIfEnterKeyPressed function because the event would be of different type than what openLink expects (KeyboardEvent vs GestureResponderEvent). So I removed event.preventDefault() call from openLink and now I don't need to pass any arguments to openLink() because event.preventDefault() is being called from openLinkOnTap() and openLinkOnEnterKey. LMK if that makes sense. The only logic change here is that I technically changed the order of when event.preventDefault() is called in openLinkOnEnterKey:
Is:
const openLinkOnEnterKey: KeyboardEventHandler = (event) => {
event.preventDefault();
if (event.key !== 'Enter') {
return;
}
openLink();
};
How it should be to be exactly the same as in .js file:
const openLinkOnEnterKey: KeyboardEventHandler = (event) => {
if (event.key !== 'Enter') {
return;
}
event.preventDefault();
openLink();
};
There was a problem hiding this comment.
I'll move event.preventDefault() below the early return.
|
|
||
| type TextLinkProps = { | ||
| /** Link to open in new tab */ | ||
| href: string; |
There was a problem hiding this comment.
href is optional
| href: string; | |
| href?: string; |
There was a problem hiding this comment.
Should it be optional?
There was a problem hiding this comment.
For me it makes more sense to have href required for a LinkText component, wdyt?
There was a problem hiding this comment.
i.e. FormAlertWrapper doesn't have href
There was a problem hiding this comment.
Looking at the code one of 'onPress' and 'href' should be defined:
const openLink = (event) => {
event.preventDefault();
if (props.onPress) {
props.onPress();
return;
}
Link.openExternalLink(props.href);
};My proposal is to use discriminating union @situchan @MaciejSWM:
{ onPress: type; } | { href: type; }There was a problem hiding this comment.
Implemented. I tested and it throws an error as expected when both href and onPress are defined. Nice @blazejkustra
src/components/TextLink.tsx
Outdated
| style?: TextStyle; | ||
|
|
||
| /** Overwrites the default link behavior with a custom callback */ | ||
| onPress?: () => void; |
There was a problem hiding this comment.
This is more accurate?
| onPress?: () => void; | |
| onPress?: (event?: GestureResponderEvent) => void; |
There was a problem hiding this comment.
No, onPress doesn't need arguments:
const openLink = () => {
if (onPress) {
onPress();
return;
}
Link.openExternalLink(href);
};
src/components/TextLink.tsx
Outdated
| ref={ref} | ||
| suppressHighlighting | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} |
There was a problem hiding this comment.
props.style will overwrite former style={[styles.link, style]}
There was a problem hiding this comment.
That's how it used to be 🤷♂️ seems reasonable for some use cases. Do you want me to move style={[styles.link, style]} below {...props} @situchan ?
There was a problem hiding this comment.
Maybe you missed important line in current code:
App/src/components/TextLink.js
Line 40 in b44f49e
There was a problem hiding this comment.
Oh shoot, good call. Fixing
There was a problem hiding this comment.
Wait... let's talk in slack because I'm now doing function TextLink({href, children, style, onPress, onMouseDown = (event) => event.preventDefault(), ...props} so technically this is equal to const rest = _.omit(props, _.keys(propTypes));
|
@situchan I guess all of the comments are addressed, ready for your review again |
|
Lint failing. Please take off draft when ready |
|
Also, pull main |
src/components/TextLink.tsx
Outdated
|
|
||
| type TextLinkProps = (LinkProps | PressProps) & { | ||
| /** Text content child */ | ||
| children: ReactElement; |
There was a problem hiding this comment.
You can use ChildrenProps here from src/types/utils/ChildrenProps.ts
There was a problem hiding this comment.
Actually after this comment it is irrelevant. You can fix it directly in TextProps 👍
type TextProps = RNTextProps &
ChildrenProps & {
/** The color of the text */
color?: string;
/** The size of the text */
fontSize?: number;
/** The alignment of the text */
textAlign?: 'left' | 'right' | 'auto' | 'center' | 'justify';
/** The family of the font to use */
family?: keyof typeof fontFamily;
};
src/components/TextLink.tsx
Outdated
| onMouseDown?: MouseEventHandler; | ||
| }; | ||
|
|
||
| function TextLink({href, onPress, children, style, onMouseDown = (event) => event.preventDefault(), ...props}: TextLinkProps, ref: ForwardedRef<RNText>) { |
There was a problem hiding this comment.
| function TextLink({href, onPress, children, style, onMouseDown = (event) => event.preventDefault(), ...props}: TextLinkProps, ref: ForwardedRef<RNText>) { | |
| function TextLink({href, onPress, children, style, onMouseDown = (event) => event.preventDefault(), ...rest}: TextLinkProps, ref: ForwardedRef<RNText>) { |
src/components/TextLink.tsx
Outdated
| ref={ref} | ||
| suppressHighlighting | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} |
There was a problem hiding this comment.
I just noticed that props is an empty object:
Most likely we need to extend TextLinkProps with TextProps (export it from src/components/Text.tsx)
type TextLinkProps = (LinkProps | PressProps) & TextProps;
|
@situchan friendly bump :) |
|
Seems like you forgot to deprecate TextLink.js |
|
@MaciejSWM friendly bump |
|
Thx for bump, ready now |
| return; | ||
| } | ||
|
|
||
| Link.openLink(props.href, environmentURL); |
There was a problem hiding this comment.
Any reason for deprecating environmentURL?
There was a problem hiding this comment.
please apply changes from #30664
|
Please pull main. |
|
@situchan main pulled. Testing steps updated. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemchrome.moviOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25023 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
✋ 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/jasperhuangg in version: 1.4.10-0 🚀
|
|
🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.4.10-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.10-1 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.10-1 🚀
|
Details
The component that is being migrated is marked in red:
Fixed Issues
$ #25023
PROPOSAL:
Tests
Verify that no errors appear in the JS console
Verify that you can click on the link with your mouse and with a finger and that it opens the link
Verify that you can highlight the link and then hit Enter and that it opens the link
Do the steps above with the following component that uses
hrefon the login page:By logging in, you agree to the[ Terms of Service ](https://use.expensify.com/terms)and[ Privacy](https://use.expensify.com/privacy).Do the steps above with the following component that uses
onPressin Adding a Card screenPlease fix the errors in the form before continuing.- on press it should move focus to the first input field that threw an errorOffline tests
You should still be able to open the link even in offline mode.
QA Steps
TestsPR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2023-11-06.at.19.38.56.mov
Android: mWeb Chrome
Screen.Recording.2023-11-06.at.19.01.27.mov
iOS: Native
Screen.Recording.2023-11-06.at.19.20.39.mov
iOS: mWeb Safari
Screen.Recording.2023-11-06.at.19.01.27.mov
MacOS: Chrome / Safari
Screen.Recording.2023-11-06.at.18.48.11.mov
MacOS: Desktop
Screen.Recording.2023-11-06.at.19.01.51.mov