Fix and improve autocomplete popup size #2919
Conversation
borisyankov
force-pushed
the
autocomplete-fix
branch
from
c41e1ac
to
3d537c4
Compare
Fixes zulip#2917 Several previous changes regressed the styling of the Popup. The ideal size of the popup can be described as: 'the whole screen sans the keyboard and with some padding around'. The lack of CSS `position` prop value of `fixed` being supported, and the lack of React portals (another option) support makes implementing this tricky and requires a lot of rework. This commit finds a good middle ground - use a max height of half the screen height minus the compose box's height.
Make the layout styling of Popup component more consistently: * layout using material design padding/margin values * 'bottom' styling is not needed * 'shadowOpacity' and 'shadowRadius' to distinguish the popup better from the background
borisyankov
force-pushed
the
autocomplete-fix
branch
from
3d537c4
to
c58e689
Compare
| return ( | ||
| <View style={[styles.wrapper, { marginBottom }]}> | ||
| <View style={[styles.wrapper, { height: height / 2 - marginBottom, marginBottom }]}> |
There was a problem hiding this comment.
(Before we had it as 1/4 of height, to tackle landscape scenario too b4cd7c8)
|
For me this results in some bad mis-layout as soon as I focus the compose box: That's on an emulated Pixel 2 XL, on Android 8. Perhaps the issue is that the More broadly, even if we find a way to do this that seems to work, I'd worry about edge cases where we don't get re-rendered -- even if we find a way that it's clearly documented should work, this feels like an area where RN is likely to have bugs in edge cases, because at least on Android it's a notoriously tricky area to get right. (What if the user enters split-screen mode? Or leaves it? Or moves their focus between split screens? Or if the keyboard itself aborts, or changes its height?) So I'd be more confident if we can find a solution that doesn't involve our code having to find out the height. |
|
Ah, playing with the Pulling it up to be a sibling will require some tricky rewiring of how the autocomplete communicates with the input fields, as it'll be one layer farther away. I think that's totally doable, but it'll be less work if we wait to do it until we've de-duplicated ComposeBox again -- which we had a previous effort at in #2886, but now is probably best done after upgrading to RN v0.57, which in turn will follow #2788 / #2789 upgrading to v0.56. Meanwhile, we should do something to make the autocomplete at least not look broken. How about setting its height to a fixed number of rows -- like 5? That should be an improvement over the status quo. |
gnprice
changed the title
|
@borisyankov and I chatted a bit about this today.
|
|
As always for a UI change, some screenshots make a key piece of the PR. For this one, I took some just now. These are for just the smaller (second) commit on its own. Before: After: |
|
Merged that piece as 69ece87. Thanks @borisyankov ! |
Fixes #2917
Makes the sizing of the autocomplete popup more consistent and logical.