wordpress-mobile / gutenberg-mobile

Mobile version of Gutenberg - native iOS and Android
GNU General Public License v2.0
242 stars 55 forks source link

Inserter shows wrong layout with Display Zoom enabled #1404

Closed koke closed 4 years ago

koke commented 4 years ago

The inserter currently doesn't support scrolling, which has been fine for now but it won't be for long. Besides that, when you enable Display Zoom, it shows up with too much padding making things worse.

Tested on an iPhone 8 iOS 12.4.1:

  1. Go to iOS Settings > Display & Brightness > Display Zoom > Set Zoomed
  2. Open gutenberg and try insert a block

In the latest TestFlight build (13.3):

IMG_E28C2C279DFC-1

In the current development builds with three more blocks (code, group, and media-text) it starts to not fit on the screen:

IMG_61EC653DADD0-1

Even in the TestFlight version, things get out of hand if you set the Dynamic Type size to the largest accessibility setting allowed:

IMG_9626BA1B3DAC-1

lukewalczak commented 4 years ago

set zoom

@koke What do you think about setting maxHeight on BottomSheet and replace View in favour of ScrollView inside?

maxHeight should has a value: window height - navbar - statusbar?

the largest accessibility setting allowed

Using proposed solution and place block one above the other.

koke commented 4 years ago

That sounds like a good approach. One thing to watch for is to avoid things being scrollable until the contents are larger than the screen. The right interactions between the scrolling and dragging down to close the sheet might be tricky, but ScrollView feels like the reasonable starting point.

Also, another issue that wasn't obvious here is that on "regular zoom", the inserter has 3 columns of items, but here it has only 2. I think that unless you use one of the huge font sizes, we should be able to still fit 3 columns on the screen by having less padding on each button.

lukewalczak commented 4 years ago

I think that unless you use one of the huge font sizes, we should be able to still fit 3 columns on the screen by having less padding on each button.

Would you like to use allowFontScaling={false} on the label or do you mean something different?

koke commented 4 years ago

No, I don't think that would fix the issue. From what I understand, display zoom doesn't adjust font sizes, but it's like setting a lower resolution on the display so things appear bigger. Here's a comparison of both modes with the same (default) font size:

zoom-compare

What I think it's happening is that each icon has a fixed width, which is assuming a certain minimum window width, and it's that layout code that should be adapted. If you look at those screenshots, the buttons are what's too big to fit 3 in a row, but I think the labels would still fit without modification.

lukewalczak commented 4 years ago

Go to iOS Settings > Display & Brightness > Display Zoom > Set Zoomed

I cannot find Set Zoomed option. Is it correct 🤔?

@koke Could you please specify the general expectations of that issue?

koke commented 4 years ago

Interesting, it seems it's only available on some phone models: https://support.apple.com/guide/iphone/magnify-the-screen-iphd6804774e/ios

I can't make sense of why those are the ones supported but I can't find the setting on my iPhone XS on iOS 13 either:

Screen Shot 2019-10-30 at 08 55 39

I'm not sure if this can be tested without an actual device in that list 😞

koke commented 4 years ago

Maybe we can achieve a similar effect by setting the width of the bottom sheet to 80-90% for testing?

lukewalczak commented 4 years ago

I think we can decrease the width to simulate the issue, and set blocks in 2 columns instead of 3. Actually I'm wondering what do we want to achieve in general when text larger accessibility sizes are ON. Do we want to fit all blocks in one column set one below the other?

koke commented 4 years ago

Do we want to fit all blocks in one column set one below the other?

That seems like the best option to me. @iamthomasbishop what do you think?

Thinking about this in terms of flexbox:

iamthomasbishop commented 4 years ago

Each button should be able to shrink so we can fit 3 columns in lower resolution. Their min-width should keep at least enough horizontal padding as they have vertical padding.

That sounds right @koke. Normally, the gutters and margins between columns is 16px, but we can narrow down the gutters (spaces in between the 2 columns) to 8px if we need to. Vertical padding could probably stay the same (12px below each row, IIRC?).

Text should not shrink though (although it shouldn't go off-screen), and would force elements to wrap to the next row.

👍. This was the one thing I was going to recommend we keep an eye on. Let's make sure wrapping looks okay – probably force a line-height of between 1 and 1.2

lukewalczak commented 4 years ago

Actually I've managed to fit it properly, however I'm still working over the right interactions between the scrolling and dragging down (unblocked the scroll in the FlatList and set the maxHeight).

I checked the scrollable example from the react-native-modal and currently I'm investigating why it doesn't work properly in our project, but on snack it works as expected.

Currently, I'm not able to achieve closing the bottom sheet by swiping down, only closing by dragging the indicator works.

Let me know wdyt @koke , @iamthomasbishop

iamthomasbishop commented 4 years ago

set the maxHeight

Can we set initial maxHeight to 50% of the viewport height? I think we'll need to do additional work to extend the functionality of the sheet component to accommodate more dynamic scroll behaviors, but for now, setting a 50% maxHeight and making it scrollable would be fine.

Regarding large font settings, I think this is about as good as it's going to get unless we wanted to dynamically switch the grid to a 2-column layout instead of 3 in this type of case (which I imagine isn't desirable).

Currently, I'm not able to achieve closing the bottom sheet by swiping down, only closing by dragging the indicator works.

I would consider this a blocker.

lukewalczak commented 4 years ago

Can we set initial maxHeight to 50% of the viewport height?

I was going to ask you what should be the maxHeight, so I have an answer. I managed to make it scrollable and it was smooth, but as mentioned I faced the issue with closing the bottom sheet by swiping down.

Regarding large font settings, I think this is about as good as it's going to get unless we wanted to dynamically switch the grid to a 2-column layout instead of 3 in this type of case (which I imagine isn't desirable).

Actually the number of columns is calculated on the basis of screenWidth - (itemWidth+ itemPaddingLeft/Right).

iamthomasbishop commented 4 years ago

Is there anything else that needs to be done to solve this issue? The sooner we can ship a fix the better, I've also noticed this issue on Android (Pixel 4) when the Display Size is set to Large.

image

lukewalczak commented 4 years ago

Hello Thomas! I have prepared a working version for ios, however I was struggling with properly working swiping down mechanism on Android. Definitely, I have to focus more on it since we are adding more and more blocks which are displayed within BottomSheet and I'll back to that issue after my Christmas break. Just out of curiosity, what do you think about horizontal scrolling blocks liss instead of vertical?

iamthomasbishop commented 4 years ago

Sounds good regarding timing! Thanks for the update.

Just out of curiosity, what do you think about horizontal scrolling blocks liss instead of vertical?

I would prefer to stay with vertical, for a few reasons:

koke commented 4 years ago

I've been running into similar issues while working on #1466. The library we use (react-native-modal) seems to support this but you need to make the scroll view and the bottom sheet talk to each other, as you did in https://github.com/WordPress/gutenberg/pull/18574. But in the case of template previews, we need to make that work with a BlockList, so we shouldn't make it part of the BottomSheet directly.

This week, I kind of made it work, but the scrolling is still not feeling as good as it should, and react-native-modal seems to have been more designed to show alerts than a bottom sheet. I'm curious about this other library react-native-reanimated-bottom-sheet, and it might be interesting to give it a try.

lukewalczak commented 4 years ago

I was trying to play a bit with that lib, but the main disadvantage is that we will have to add 3 three libs at the end (react-native-gesture-handler, react-native-reanimated and react-native-reanimated-bottom-sheet). What's more I had some issues with KeyboardAvoidingView with inputs within BottomSheet

iamthomasbishop commented 4 years ago

@koke that library looks really great. @lukewalczak's concerns, esp about inputs within the component, would be worrisome to me considering we will rely heavily on complex controls in the sheets. But if we can avoid those issues I think something like this would be really great.

designsimply commented 4 years ago

This came up in WPiOS 14.1 beta testing at https://github.com/wordpress-mobile/WordPress-iOS/issues/13304. More blocks have made the issue more relevant!

lukewalczak commented 4 years ago

Currently, we are supporting more and more blocks and that issue is more expanded - it's not related only to zoom but even on the horizontal mode we are not able to choose some block.

This is a temporarily workaround. For more details, please check that comment