-
Notifications
You must be signed in to change notification settings - Fork 3.5k
feat: interactive keyboard dismissal #65517
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
feat: interactive keyboard dismissal #65517
Conversation
| <KeyboardGestureArea | ||
| style={styles.flexGrow1} | ||
| offset={90} | ||
| interpolator="ios" | ||
| textInputNativeID="composer" | ||
| > |
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 wanted to highlight, that usage of this component will automatically enable interactive dismissal on Android (so we have to test that platform carefully as well).
Also if you discover any kind of bugs/undesired behavior, don't hesitate to remove it and test the implementation on iOS without this component. If KeyboardGestureArea produces any bugs, then let me know and I'll try to fix them!
(posting it here because KeyboardGestureArea relatively new component on iOS and may have some bugs 😅)
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.
Sure, it's working alright on android so far!
| <KeyboardGestureArea | ||
| style={styles.flexGrow1} | ||
| offset={90} | ||
| interpolator="ios" | ||
| textInputNativeID="composer" | ||
| > |
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 we shouldn't hardcode it? (correct me if I'm wrong)
When the multiline input grows I think it changes its size so we should reflect those changes here?
(but again, correct me if I'm wrong, maybe the approach with growing input has been re-worked recently and now it doesn't change its height if you type a lot of text)
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.
Yeah, i changed it to grow according to the composer height :)
roryabraham
left a comment
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.
am I correct in thinking that this is the PR in which we should remove android:windowSoftInputMode="adjustResize" in both NewDot standalone and HybridApp?
|
Hey @roryabraham sorry about the delay - i'm not sure what android:windowSoftInputMode="adjustResize" does, but i can take a look. |
|
Disclaimer: It's been a long time since we started working on the react-native-keyboard-controller migration, and I'm writing from what I recall here. My recollection may be flawed. You and @kirillzyusko should investigate further. If I'm not mistaken,
Furthermore, if memory serves, |
|
@roryabraham we enabled edge-to-edge and kept using So at this point of time all keyboard avoiding is handled by react-native-keyboard-controller (mostly by its KeyboardAvoidingView component), not by OS anymore. So now we can write cross-platform code that will work identically everywhere, and interactive keyboard dismissal can be implemented on Android too 👀 |
|
@kirillzyusko legend |
392dc16 to
2c0b3e9
Compare
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
@parasharrajat here for a review, please give it a go and report any bugs you find! thanks! |
|
I will test it shortly... |
|
Looks like we are still working on some issues and in the process of discussing the implementation. I will keep a watch on this PR as we progress. Feel free to seek help from me anytime if needed. |
04d87d0 to
183f8df
Compare
chrispader
left a comment
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 looks pretty good already! Left some comments around the new implementation that we discussed yesterday.
| lastScrollEvent.current = timestamp; | ||
| }; | ||
|
|
||
| useAnimatedReaction( |
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 do we have to manually trigger onScroll here with an animated reaction? Can't we just pass the callback as a prop down to BaseInvertedFlatList -> KeyboardDismissableFlatList which then calls it it alongside the keyboard dismissal logic in a joint callback?
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 don't really like the idea of having a separate manual trigger for the calling the onScroll callback
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 can be done, i thought about just leaving this here since it was already here
| @@ -0,0 +1,127 @@ | |||
| import PropTypes from 'prop-types'; | |||
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.
Do we really need to have the keyboard dismissal logic in a context? I think this might cause some unnecessary re-renders. Can't we have just move this logic to a hook and then use it in KeyboardDismissableFlatList? This is not used across multiple components, right?
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 was a hook and it is actually used across some components. I decided to write a context because I wanted to keep the scroll and insets implementation closer to the KeyboardDismissableFlatList component, and other components need access to the values that are updated by this component.
src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx
Outdated
Show resolved
Hide resolved
| }; | ||
| }); | ||
|
|
||
| const onLayoutInternal = useCallback( |
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.
see comment in ReportActionCompose
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 one is actually needed to track the composer height on this component
b8e9059 to
3d938f9
Compare
|
@roryabraham 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] |
|
I will start review now. |
2dcb0d7 to
a64666a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks Chris |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…atedKeyboardHandler``
|
@roryabraham @trjExpensify @parasharrajat i would like to split this PR up into 2 smaller PRs, to prevent this PR from being reverted because of regressions and make review and testing easier:
Does that sound good for you? |
|
@chrispader I agree. |
… restructure type
|
@roryabraham @trjExpensify @parasharrajat I've went ahead and split this up into two PRs: |
Explanation of Change
This PR aims to add interactive keyboard dismissal (or pull down to close keyboard) capabilities to the composer screen, specifically the ReportActionsList component and MoneyRequestReportActionsList. In order to do so, it adds:
useKeyboardHandlerand theKeyboardGestureAreacomponent fromreact-native-keyboard-controller. The iOS implementation animates the list insets using the keyboard height and the scroll position as parameters, as this is the recommended approach fromreact-native-keyboard-controller.KeyboardGestureAreacomponent is used, but the gesture does not work on web.Fixed Issues
$ #54270
PROPOSAL: Adding the
react-native-keyboard-controllerrecommended setup to achieve the interactive keyboard dismissal.Tests
For Android and iOS
For other platforms
The pull down to dismiss is not applicable, but all steps except 4 and 6 from the android and ios list can be tested here.
PS: I was not able to test on iOS Safari, but i'm guessing it behaves the same as iOS chrome.
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)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.mov
Android: mWeb Chrome
chrome.android.mp4
iOS: Native
ios.mov
iOS: mWeb Chrome
chrome.ios.mov
MacOS: Chrome / Safari
web.desktop.mov
MacOS: Desktop
web.desktop.mov