Fix exit survey response page validation runs when open#51516
Conversation
There was a problem hiding this comment.
Please update the file name to index.os.ts to reflect changes based on updateMultilineInputRange
src/libs/InputUtils/index.web.ts
Outdated
There was a problem hiding this comment.
Please update the file name to index.ts to reflect changes based on updateMultilineInputRange
There was a problem hiding this comment.
I don't think we should follow updateMultilineInputRange. Also, index.ts as the native file allow us to pass the ref without changing the ref type.
The ref type of useAutoFocusInput is TextInput. (It's actually should be AnimatedTextInputRef)
App/src/hooks/useAutoFocusInput.ts
Lines 9 to 12 in 652d2ff
So, passing the ref to moveSelectionToEnd, for example, won't complain about the type because moveSelectionToEnd in index.ts accepts TextInput.
That's why I reverted changing the type to AnimatedTextInputRef because I realized it's not needed.
There was a problem hiding this comment.
Ok I suggested change the filename the same with updateMultilineInputRange to keep the changes to ensure consistent behavior across platforms after change but look like the input on android native doesn't have scrollTop & setSelectionRange so we can keep the current file name for index & index.web
src/libs/InputUtils/index.web.ts
Outdated
| @@ -0,0 +1,11 @@ | |||
| function scrollToBottom(input: HTMLInputElement) { | |||
There was a problem hiding this comment.
We can create function type the same updateMultilineInputRange/types.ts to avoid different parameter on each platforms
There was a problem hiding this comment.
If we do that, then we need to check for scrollTop again because the input could be HTMLInputElement | TextInput. They have different type because they are different.
if ('scrollTop' in input) ...
There was a problem hiding this comment.
Yes please do that, then we can remove disable type check for InputUtils/index.ts
There was a problem hiding this comment.
Sorry, not sure if I get it.
// eslint-disable-next-line @typescript-eslint/no-unused-vars
I added this because the param in the index.ts file is unused. Let me know if I missed what you mean.
There was a problem hiding this comment.
After create function type we can do like that to avoid disable eslint
// types.ts
export type ScrollToBottom = (input: HTMLInputElement | TextInput) => void;
// index.ts
const scrollToBottom: ScrollToBottom = () => {};There was a problem hiding this comment.
Done. I renamed the file too because the function now accept HTMLInputElement | TextInput
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-27.at.23.27.03.mp4Android: mWeb ChromeScreen.Recording.2024-10-27.at.23.28.12.mp4iOS: NativeScreen.Recording.2024-10-27.at.23.19.27.mp4iOS: mWeb SafariScreen.Recording.2024-10-27.at.23.18.18.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-27.at.23.15.36.mp4MacOS: DesktopScreen.Recording.2024-10-27.at.23.15.57.mp4 |
|
LGTM |
neil-marcellini
left a comment
There was a problem hiding this comment.
The code looks good and it seems to test well. Thanks
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.0.55-0 🚀
|
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.0.55-10 🚀
|
Details
When we open the exit survey page, the validation is immediately runs because the input is blurred.
Fixed Issues
$ #50962
PROPOSAL: #50962 (comment)
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso 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
android.mweb.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
ios.mweb.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4