Add scroll ability relative to caret position when keyboard is open#2
Add scroll ability relative to caret position when keyboard is open#2
Conversation
This includes orientation change etc. We don't need to get different values for landscape/portrait modes from outside anymore.
This sometimes happens if device orientation changes
etoledom
left a comment
There was a problem hiding this comment.
Tested on
wordpress-mobile/gutenberg-mobile#461 (review) and it's working super good! 🎉
There is just one detail that is bugging me:
This implementation uses the RCTUIManager extension defined in react-native-gutenberg-bridge. This means that this 3rd party library became dependent of react-native-gutenberg-bridge.
Even though this is iOS only (for now), the idea that this library requires to be used just together with our bridge feels unusual, and it would be nice to be able to give back our improvements to the community.
I see the advantage of simplicity, since this is a fork we can mold it to serve our needs, and that should probably be our focus, but sadly nobody else will be able to enjoy these improvements in any other context other than gutenberg-mobile.
In the other hand, making these improvements usable for anybody in any context would mean an extra effort not really needed for gutenberg-mobile, specially not in this specific PR.
Personally I'm fine with this as it is if we make a note to restructure it in the future. I don't think it is worth it the effort right now. What do you think @pinarol ?
I'd like to ask for @hypest 's opinion in the topic too if possible.
lib/KeyboardAwareHOC.js
Outdated
| } | ||
| } else { | ||
| // On android, the system would scroll the text input just | ||
| // above the keyboard so we just neet to scroll the extra |
There was a problem hiding this comment.
Small typo
| // above the keyboard so we just neet to scroll the extra | |
| // above the keyboard so we just need to scroll the extra |
There was a problem hiding this comment.
Cool!
Just being curious, does the GitHub Suggested change thing works?
There was a problem hiding this comment.
Suggested change works actually, it just sometimes doesn't catch my attention :)
There was a problem hiding this comment.
I fixed that manually ;)
|
That's a perfectly valid concern @etoledom it is best to keep the Javascript code together with its native side and contribute back to the community. The thing is I already changed this file a lot for our needs and there are other changes coming. On some point we can consider moving the javascript code to gutenberg-mobile considering it is just 1 file and a super small interface file. I'll investigate what needs to be done to put the iOS code here, it'll be the first native code in this repo I don't know the effort to be honest. |
|
It was easier than I though @etoledom I'll update the gutenberg-mobile PR accordingly |
|
late note: I remembered why I put this is to our bridge in the first place @etoledom because in the first solution the category was importing RNTAztecView, then I had updated it to depend on UIKit only. |
There was a problem hiding this comment.
Thank you for taking care of that change!
Being self-contained looks better and is good for peace of mind. 😛
I tested it carefully on WPiOS and the example app, iPhone, iPad and Android device, following the steps described here and it's working great!
Very good job @pinarol !
Co-Authored-By: pinarol <pinarolguc@gmail.com>
So true! Thanks for the detailed review @etoledom 👍 |
To fix:
wordpress-mobile/gutenberg-mobile#436
wordpress-mobile/gutenberg-mobile#432
This PR adds the ability to scroll to caret position when keyboard is open and adds ability to check if caret will be already visible once keyboard is open, thus, we don't need to scroll. Until this PR the automatic scrolling was only scrolling to the end of the focused component and that was causing problems when we have a long text.
After this PR the scrolling decision is given this way:
Scrolling to caret is only available for components who conform to UITextInput, this includes RNTAztecView. React native's text input doesn't provide us the caret position and hides the inner TextView from us so we can't apply scroll to caret behavior in React Native's text input, they will continue to scroll to the bottom of the component. In the future we can replace that with RNTAztecView to achieve same thing.
Why did I add the check for "Is caret at the last line of text input?"
Because automatic scroll didn't look consistent enough otherwise. When you tap some of the lines it was scrolling to caret and caret stayed just above the keyboard as expected, but, when you tap 1 line below for example, the auto scroll could move the caret to position at the top of the page because it performed
scroll to the end of componentthis time.Review recommendation
I recommend checking some commits separately because some of the changes are just extracting the existing code to a method so those can be differentiated from new code.
To Test