-
-
Notifications
You must be signed in to change notification settings - Fork 643
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
ChatScreen: Fix blank strip between compose box and keyboard on iOS #5564
Conversation
src/chat/ChatScreen.js
Outdated
keyboardVerticalOffset={ | ||
// This is a slight misuse of this prop: this value is not (to quote | ||
// from the jsdoc) "How much the top of `KeyboardAvoider`'s layout | ||
// *parent* is displaced downward from the top of the screen." That | ||
// value would be zero. | ||
// | ||
// This addresses an issue where the strip that covers the bottom | ||
// inset would scoot up above the keyboard with the rest of the | ||
// compose box when the keyboard opens, leaving a blank strip of | ||
// wasted space ( #3370). We haven't yet found a better way to avoid | ||
// this, while still letting `ComposeBox` control how it occupies | ||
// the inset (like how our header components occupy the top inset). | ||
-useSafeAreaInsets().bottom | ||
} |
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.
Hmm fascinating.
What does this look like when the keyboard isn't open? Does it cause the compose box to be lowered into the inset area, where it's obscured at the bottom? On its face it seems like it would.
It looks like this would become our one use of this keyboardVerticalOffset
prop. If something like this is right, then perhaps the jsdoc on it isn't right and should be edited? It looks like we added that prop in 70eca07 / #4683 and have never yet used it in main. (It subsequently appeared in #4893, in a previous revision of this same commit.)
583a5ca
to
281bcdc
Compare
Thanks for the review, and for finding a better solution just now in discussion in the office! 🙂 Revision pushed. |
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.
Thanks! Looks good -- small comment-comments below.
* Use this when a child component uses SafeAreaView to pad the bottom | ||
* inset, and you want the open keyboard to cover that padding. | ||
* | ||
* (SafeAreaView has a bug where it applies a constant padding no matter | ||
* where it's positioned onscreen -- so in particular, if you ask for | ||
* bottom padding, you'll get bottom padding even when KeyboardAvoider | ||
* pushes the SafeAreaView up and out of the bottom inset.) |
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 seems very clear, thanks!
compensateOverpadding={ | ||
// We let the compose box pad the bottom inset. | ||
showComposeBox |
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.
Let's have a comment at the use of SafeAreaView
in ComposeBox
, too, saying that when the keyboard is open it's compensated for here.
src/common/KeyboardAvoider.js
Outdated
keyboardVerticalOffset={ | ||
keyboardVerticalOffset + (compensateOverpadding ? -bottomInsetHeight : 0) |
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.
At this spot we're abusing the keyboardVerticalOffset
prop of KeyboardAvoider
-- what we're doing doesn't match its intended interface. So a comment to acknowledge that would be helpful, letting the reader know the mismatch is intentional and they're not just missing something. Like this, say:
keyboardVerticalOffset={ | |
keyboardVerticalOffset + (compensateOverpadding ? -bottomInsetHeight : 0) | |
keyboardVerticalOffset={ | |
keyboardVerticalOffset | |
// A negative term here reduces the bottom inset we use for the child, | |
// when the keyboard is open, so that the keyboard covers its bottom padding. | |
+ (compensateOverpadding ? -bottomInsetHeight : 0) |
281bcdc
to
9ed6fd3
Compare
Thanks for the review! Revision pushed. |
Cool, the screenshot looks good to me! |
9ed6fd3
to
ee6630e
Compare
Merging with that fix, thanks for the review! |
ee6630e
to
cbdc035
Compare
Before/after:
Fixes: #3370