-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[TS migration] Migrate TextLink to Typescript #30907
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
Merged
jasperhuangg
merged 30 commits into
Expensify:main
from
software-mansion-labs:ts-migration/TextLink
Dec 7, 2023
Merged
Changes from all commits
Commits
Show all changes
30 commits
Select commit
Hold shift + click to select a range
d291618
Migrate to TS
MaciejSWM 1aada2b
Merge branch 'main' into ts-migration/TextLink
MaciejSWM cda8b73
Relative imports; optional params; types
MaciejSWM 383db18
Remove unused import
MaciejSWM f7ea0fe
Import ordering; Ignore no spreading
MaciejSWM 71fe175
Extract logic to openLink method
MaciejSWM 4c0a808
Missing semicolon
MaciejSWM 659b4cc
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 89f3905
Better ref handling
MaciejSWM af92086
More preventDefault after early return
MaciejSWM d4802db
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 0ded84a
Make href optional
MaciejSWM 5304f4d
More compact else/if syntax
MaciejSWM df2d98a
Use discriminating union type for link and onPress
MaciejSWM eadc4a2
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 65e05f9
Prettier
MaciejSWM 1b89761
Better type for children + export type
MaciejSWM bc41d43
Extend TextProps; rename props->rest
MaciejSWM 3b4ceaf
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 3e26c7b
Fix merge - apply changes from olx .js file
MaciejSWM 1d5c0af
Lint
MaciejSWM 732efed
Extend ChildrenProps
MaciejSWM 95ab86a
Lint
MaciejSWM 0e109d4
Merge branch 'main' into ts-migration/TextLink
MaciejSWM cfc444c
Use TextStyles' textAlign type
MaciejSWM 82d1eb7
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 1a8f7ad
Drop js file
MaciejSWM d95966a
Restore environmentURL
MaciejSWM ddf0d4e
Merge branch 'main' into ts-migration/TextLink
MaciejSWM 83504ed
Merge branch 'main' into ts-migration/TextLink
MaciejSWM File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import React, {ForwardedRef, forwardRef, KeyboardEventHandler, MouseEventHandler} from 'react'; | ||
| import {GestureResponderEvent, Text as RNText, StyleProp, TextStyle} from 'react-native'; | ||
| import useEnvironment from '@hooks/useEnvironment'; | ||
| import useThemeStyles from '@styles/useThemeStyles'; | ||
| import * as Link from '@userActions/Link'; | ||
| import CONST from '@src/CONST'; | ||
| import Text, {TextProps} from './Text'; | ||
|
|
||
| type LinkProps = { | ||
| /** Link to open in new tab */ | ||
| href: string; | ||
|
|
||
| onPress?: undefined; | ||
| }; | ||
|
|
||
| type PressProps = { | ||
| href?: undefined; | ||
|
|
||
| /** Overwrites the default link behavior with a custom callback */ | ||
| onPress: () => void; | ||
| }; | ||
|
|
||
| type TextLinkProps = (LinkProps | PressProps) & | ||
| TextProps & { | ||
| /** Additional style props */ | ||
| style?: StyleProp<TextStyle>; | ||
|
|
||
| /** Callback that is called when mousedown is triggered */ | ||
| onMouseDown?: MouseEventHandler; | ||
| }; | ||
|
|
||
| function TextLink({href, onPress, children, style, onMouseDown = (event) => event.preventDefault(), ...rest}: TextLinkProps, ref: ForwardedRef<RNText>) { | ||
| const {environmentURL} = useEnvironment(); | ||
| const styles = useThemeStyles(); | ||
|
|
||
| const openLink = () => { | ||
| if (onPress) { | ||
| onPress(); | ||
| } else { | ||
| Link.openLink(href, environmentURL); | ||
| } | ||
| }; | ||
|
|
||
| const openLinkOnTap = (event: GestureResponderEvent) => { | ||
| event.preventDefault(); | ||
|
|
||
| openLink(); | ||
| }; | ||
|
|
||
| const openLinkOnEnterKey: KeyboardEventHandler = (event) => { | ||
| if (event.key !== 'Enter') { | ||
| return; | ||
| } | ||
| event.preventDefault(); | ||
|
|
||
| openLink(); | ||
| }; | ||
|
|
||
| return ( | ||
| <Text | ||
| style={[styles.link, style]} | ||
| role={CONST.ACCESSIBILITY_ROLE.LINK} | ||
| href={href} | ||
| onPress={openLinkOnTap} | ||
| onKeyDown={openLinkOnEnterKey} | ||
| onMouseDown={onMouseDown} | ||
| ref={ref} | ||
| suppressHighlighting | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...rest} | ||
| > | ||
| {children} | ||
| </Text> | ||
| ); | ||
| } | ||
|
|
||
| TextLink.displayName = 'TextLink'; | ||
|
|
||
| export default forwardRef(TextLink); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
href is optional
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.
Should it be optional?
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.
For me it makes more sense to have href required for a LinkText component, wdyt?
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.e. FormAlertWrapper doesn't have href
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.
Looking at the code one of 'onPress' and 'href' should be defined:
My proposal is to use discriminating union @situchan @MaciejSWM:
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.
Nice!
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.
Implemented. I tested and it throws an error as expected when both
hrefandonPressare defined. Nice @blazejkustra