-
-
Notifications
You must be signed in to change notification settings - Fork 137
[W.I.P] KeyboardAwareScrollView Rework To work with multilines #901
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
Conversation
…g for Android edge cases
…ions on drag start
Demo.mp4 |
|
@kirillzyusko lmk what you think |
| scrollEventThrottle={16} | ||
| onContentSizeChange={onContentSizeChange} | ||
| onLayout={onScrollViewLayout} | ||
| onScroll={scrollHandler} |
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.
| () => scrollY.value, | ||
| (current) => { | ||
| scrollTo(scrollViewAnimatedRef, 0, current, false); | ||
| }, |
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.
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 can change this. I liked the effect for inputs that don't fit inside the visible area. Without this the user would need to scroll themselves or beginning to type for the move to happen.
I can change it so the scroll stops at the top of the input instead? and then follows from the caret at bottom until it reaches the bottom of the input where it will sit if it has a maxHeight?
| scrollY.value = spring | ||
| ? withSpring(goal, { damping: 100, stiffness: 100 }, (finished) => { | ||
| log( | ||
| "Goal:", | ||
| goal, | ||
| "Current:", | ||
| scrollOffsetY.value, | ||
| "EndingPosition:", | ||
| scrollY.value, | ||
| "Distance From Goal:", | ||
| Math.abs(scrollOffsetY.value - goal), | ||
| "Finished:", | ||
| finished, | ||
| ); | ||
| isScrolling.value = false; | ||
| }) | ||
| : withTiming(goal, undefined, (finished) => { | ||
| log( | ||
| "Goal:", | ||
| goal, | ||
| "Current:", | ||
| scrollOffsetY.value, | ||
| "EndingPosition:", | ||
| scrollY.value, | ||
| "Distance From Goal:", | ||
| Math.abs(scrollOffsetY.value - goal), | ||
| "Finished:", | ||
| finished, | ||
| ); | ||
| isScrolling.value = false; | ||
| }); | ||
| }; |
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 need this? We should perform scroll based on a keyboard position? So we should scroll from onMove handlers.
Plain scroll can happen when text input grows, but in this case scrollTo(..., true) with default animation works also pretty well 🤔
| {new Array(10).fill(0).map((_, i) => ( | ||
| {new Array(5).fill(0).map((_, i) => ( |
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.
If you modify code example in such way there is 100% guarantee that e2e tests will fail, because they expect to have certain amount of inputs.
I'd suggest to revert these changes (I understand that it's more convenient not scroll through entire form, but you can keep these changes locally and not commit them to the repository)
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.
Planned to clean this up and wasn't suppose to commit the changes to the example.
| type Props = StackScreenProps<ExamplesStackParamList>; | ||
| const snapToOffsets = [125, 225, 325, 425, 525, 625]; | ||
|
|
||
| const BIG_TEXT_TWO = `Where does it come from? |
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.
Maybe let's not repeat code and re-use variables?
But again, for playground code you can keep a local changes and not commit them to the repository.
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.
Planned to clean this up and wasn't suppose to commit the changes to the example.
|
And feedback based on video:
Overall you did a great job 💪 But certain points must be addressed I don't think I can merge PR in a current state 🤔 But if you don't want to spend too much time on going back-and-forth with review, then you can use that modified version of Also if you want we can have a conversation in discord, it should significantly improve our communication process 😊 |
|
I'm going away this weekend so have not much time but would love to jump on discord next week if that works with you @kirillzyusko 0:08-0:09 - feels off
0:13-0:15 - I think we shouldn't scroll to follow cursor position. If scroll remains in a visible area then better not scroll, since user can still see the input.
0:25-0:27 - feels like scroll is stuttering a little bit?
0:50-0:51 - why we scrolled such a big distance? I thought we always rely on selection coordinates (not on layout anymore)?
0:59-1:00 - animation feels desynchronized?
when back scroll turned off it looks like you add back scroll? And when it's enabled I don't see a back scroll?
not sure, but since you modified example it looks like snap points are broken now
|
Already on it 😎. Thanks for the great api |
|
I'll close this PR in favour of #1026 It'll be published under |
## 📜 Description Allow `KeyboardAwareScrollView` to work with inputs that covers whole screen. ## 💡 Motivation and Context Previous implementation worked incorrectly when you had a `TextInput` that gets covered by keyboard and navigation header **simultaneously**. It was happening because I didn't consider a case, when you have a very large input - intead I was relying on `TextInput` coordinates on the screen, and if it overlaps with keyboard, then we need to scroll to bottom, if it overlaps with header - scroll to top. However in many apps sometimes it's needed to have a really large input (for example Notes, Gmail, etc.) In this PR I re-worked approach and now I don't use `TextInput` layout coordinates anymore and I rely on selection coordiantes (aka cursor). The preparation work has been started in #546 but this PR fully implements new specification (not only one handler). We can not rely only on selection coordinates, because if input has `maxHeight` then `selection` coordinates can be very big (if you typed a lot and text content height exceeds `maxHeight`) - in this case to handle it properly we clamp the value to `layout.height`, so we still need to use `useReanimatedFocusedInput` hook. Closes #512 #578 #897 #937 #901 ## 📢 Changelog <!-- High level overview of important changes --> <!-- For example: fixed status bar manipulation; added new types declarations; --> <!-- If your changes don't affect one of platform/language below - then remove this platform/language --> ### Docs - mention that `bottomOffset` is a distance between keyboard and caret inside focused `TextInput`; ### E2E - update assets; ### JS - added `updateLayoutFromSelection` function; - don't rely on `input.value` coordinated directly; ## 🤔 How Has This Been Tested? Tested on iPhone 15 Pro (iOS 17.5) and Pixel 3A (API 33). Additionally tested via e2e tests. ## 📸 Screenshots (if appropriate): |iOS|Android| |---|--------| |<video src="https://github.com/user-attachments/assets/744bf413-4aca-49ed-aef8-6bd458d09c19">|<video src="https://github.com/user-attachments/assets/e09c42c1-9204-4291-9df4-3bf179a0d27a">| ## 📝 Checklist - [x] CI successfully passed - [x] I added new mocks and corresponding unit-tests if library API was changed


📜 Description
Will write up a proper description later and clean up the code.
current issue until done (Android):
The multiple line inputs always start the text cursor from the bottom on first focus
💡 Motivation and Context
📢 Changelog
JS
iOS
Android
🤔 How Has This Been Tested?
📸 Screenshots (if appropriate):
📝 Checklist