[TS migration] Migrate 'TextInput' component to TypeScript #31356
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
…s-migration/TextInput/component
…s-migration/TextInput/component
…s-migration/TextInput/component
…s-migration/TextInput/component
…s-migration/TextInput/component
src/components/TextInput/index.tsx
Outdated
| textInputRef.current?.setAttribute('name', name); | ||
| } | ||
| // @ts-expect-error We need to reassign this ref to the input ref | ||
| removeVisibilityListenerRef.current = Visibility.onVisibilityChange(() => { |
There was a problem hiding this comment.
This way you can get rid of @ts-expect-error
type RemoveVisibilityListener = () => void;
....
const removeVisibilityListenerRef = useRef<RemoveVisibilityListener>(null);
...
(removeVisibilityListenerRef.current as RemoveVisibilityListener) = Visibility.onVisibilityChange(() => {
There was a problem hiding this comment.
@VickyStash I fixed that using different approach since what you suggested was causing another error. Let me know what you think 😄
|
@fedirjh fixed! |
|
@fedirjh Would you be able to look at it today/tomorrow? This is blocking a ton of other TS issues 🙏 |
@blazejkustra Already started on it , I am blocked with this issue on native after last main merge 
|
| // Add +2 to width on Safari browsers so that text is not cut off due to the cursor or when changing the value | ||
| // https://github.com/Expensify/App/issues/8158 | ||
| // https://github.com/Expensify/App/issues/26628 | ||
| <Text | ||
| style={[...props.inputStyle, props.autoGrowHeight && styles.autoGrowHeightHiddenInput(width, maxHeight), styles.hiddenElementOutsideOfWindow, styles.visibilityHidden]} | ||
| style={[ | ||
| inputStyle, |
There was a problem hiding this comment.
Why we did not destruct this style ?
There was a problem hiding this comment.
@fedirjh There is no need for spreading styles props , it will work without that
| <FormHelpMessage | ||
| isError={!_.isEmpty(props.errorText)} | ||
| isError={!errorText} |
There was a problem hiding this comment.
| isError={!errorText} | |
| isError={!!errorText} |
This is causing a bug on web , the color of the error text is not correct.
| Actual | Expected |
|---|---|
 |
 |
There was a problem hiding this comment.
Will fix that soon
There was a problem hiding this comment.
Done @fedirjh
| Animated.parallel([ | ||
| Animated.spring(labelTranslateY, { | ||
| toValue: translateY, | ||
| duration: styleConst.LABEL_ANIMATION_DURATION, |
There was a problem hiding this comment.
Won't this change affect the animation on native ?
There was a problem hiding this comment.
No, actually there is no duration option in for spring animation , so it was doing nothing
Reviewer Checklist
Screenshots/VideosAndroid: NativeCleanShot.2023-12-19.at.22.47.01.mp4Android: mWeb ChromeCleanShot.2023-12-19.at.06.08.56.mp4
iOS: NativeCleanShot.2023-12-19.at.19.42.40.mp4iOS: mWeb SafariCleanShot.2023-12-18.at.23.15.00.mp4MacOS: Chrome / SafariCleanShot.2023-12-19.at.05.52.33.mp4
MacOS: DesktopCleanShot.2023-12-19.at.05.55.15.mp4
|
…s-migration/TextInput/component
|
@kubabutkiewicz Could you please merge main ? I hope that will fix this issue #31356 (comment) . |
…s-migration/TextInput/component
|
@fedirjh done! 😄 |
|
Bump @fedirjh 😄 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25146 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
There was a problem hiding this comment.
NAB - most prop descriptions in this file are pretty weak / not descriptive. Maybe they came from somewhere else in the codebase (a.k.a. weren't added new here?) but it would be best to try to make them as clear as possible... Probably in a follow-up PR
|
|
||
| const TextInput = forwardRef((props, ref) => { | ||
| function TextInput(props: BaseTextInputProps, ref: BaseTextInputRef) { |
There was a problem hiding this comment.
NAB - Question: Why aren't we destructuring props here, but we do in other files like TextInputLabel?
| import isSelectorSupported from './isSelectorSupported'; | ||
|
|
||
| /** | ||
| * Check the input is auto filled or not | ||
| */ | ||
| export default function isInputAutoFilled(input: Element): boolean { | ||
| if (!input?.matches) { | ||
| export default function isInputAutoFilled(input: (TextInput | HTMLElement) | null): boolean { |
There was a problem hiding this comment.
NAB - Is input: (TextInput | HTMLElement) | null not funky? I would have expected input: (TextInput | HTMLElement | null) to work and look better, no? I'd highly appreciate a lesson why it's necessary we do this, if anyone has the time 😅
| autoCompleteType?: string; | ||
| }; | ||
|
|
||
| type BaseTextInputRef = ForwardedRef<HTMLFormElement | Component<AnimatedProps<TextInputProps>>>; |
There was a problem hiding this comment.
Super NAB - can we use AnimatedTextInputRef here like this?
| type BaseTextInputRef = ForwardedRef<HTMLFormElement | Component<AnimatedProps<TextInputProps>>>; | |
| type BaseTextInputRef = ForwardedRef<HTMLFormElement | AnimatedTextInputRef>; |
| const onPress = (event) => { | ||
| if (props.disabled) { | ||
| const onPress = (event?: GestureResponderEvent | KeyboardEvent) => { | ||
| if (!!inputProps.disabled || !event) { |
There was a problem hiding this comment.
NAB - Why do we need !! in front of inputProps.disabled?
|
|
||
| if (!event.isDefaultPrevented()) { | ||
| input.current.focus(); | ||
| if ('isDefaultPrevented' in event && !event?.isDefaultPrevented()) { |
There was a problem hiding this comment.
I feel like you've done this in lots of places, and forgive my ts-noobness please :D But why do we have to check 'isDefaultPrevented' in event AND also use optional chaining event?.isDefaultPrevented?
My guess is:
'isDefaultPrevented' in eventis to satisfy typescript- optional chaining is to make sure
isDefaultPreventedis not undefined / null
|
Oh @kubabutkiewicz I would also recommend you close out old comments that don't apply anymore / are resolve, as during my review I came across some open threads and wasn't sure if you'd addressed them or forgotten about them 😅 let me know what you think! |
|
oops this may have broken type check on main |
|
@Beamanator #33796 will fix this |
|
Hi @Beamanator sorry I was OOO, if there is still some problem I can take a look or answer your questions! |
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 1.4.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
|
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.21-4 🚀
|
Details
Fixed Issues
$ #25146
Tests
Offline tests
QA Steps
PR 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)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.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
android.mp4
Android: mWeb Chrome
iOS: Native
ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4