Skip to content

Conversation

@pinarol
Copy link
Contributor

@pinarol pinarol commented Dec 25, 2018

Fixes #395

Child PR needs to be merged first. After that I'll tag that commit and update the dependency to point to the tag.

Parent PR is needed for testing with WPiOS.

Implementation details

The basic problem is solved with the first commit 09cb2f0 which updates the dependency to refer to our fork of the react-native-keyboard-aware-scroll-view library.

But still a small amount of extra padding was left on iPhone X variant devices, that was resulted from the safe area inset. So the rest of the commits solves only that small amount of extra padding. If you checkout 09cb2f0 and test on iPhone X you can view that extra padding.

To Test

  • must do -> yarn install
  • yarn start
  • open example app with XCode, clean build folder

Do below steps with iPhone 8, iPhone X(or variant), iPad

  • Open a post which has a bigger content than the viewable area
  • Tap one of the text fields to open the keyboard
  • Scroll to the bottom of the page while keyboard is open
  • Verify that there are no unwanted bottom inset
  • Switch to Landscape
  • Scroll to the bottom of the page while keyboard is open
  • Verify that there are no unwanted bottom inset

WPiOS

  • To test WPiOS checkout the branch in parent PR
  • run rake dependencies
  • and repeat test steps

Android
Open example app with Android
Verify it doesn't crash <- this is actually enough
Verify tapping to input text fields makes keyboard open properly by keeping text viewable.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I wasn't sure about installing a new library (react-native-safe-area) for such a small detail, specially having the SafeArea component from Facebook.

After investigating, testing and understanding the issue and the solution, I'm quite convinced that the use of this new SafeArea library is not just necessary to fix this issue, but also will be super useful in the future. Specially thanks to this method:
SafeArea.getSafeAreaInsetsForRootView()

So sad that the "original" SafeArea component doesn't expose the paddings added to the view, or any way to get in JS the SafeArea insets.

This new react-native-safe-area will allow us to avoid embedding all our content in the SafeArea view and implement the the design with the toolbar borders going until the end of the screen (and probably will help us with other details later on too).

It's working great in both the example project and the WPiOS app ✅

Amazing job @pinarol !

@pinarol
Copy link
Contributor Author

pinarol commented Dec 27, 2018

@etoledom agreed, I wasn't also %100 sure at the beginning but react-native-safe-area gives us much more control. thanks for the detailed review 🎉

@pinarol pinarol merged commit 5640809 into develop Dec 27, 2018
@pinarol pinarol deleted the issue/395-fix-content-bottom-inset-ios branch December 27, 2018 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants