Skip to content
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

Inserter shows wrong layout with Display Zoom enabled #1404

Closed
koke opened this issue Oct 4, 2019 · 21 comments · Fixed by #1837
Closed

Inserter shows wrong layout with Display Zoom enabled #1404

koke opened this issue Oct 4, 2019 · 21 comments · Fixed by #1837

Comments

@koke
Copy link
Member

koke commented Oct 4, 2019

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
Copy link
Contributor

lukewalczak commented Oct 4, 2019

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
Copy link
Member Author

koke commented Oct 4, 2019

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
Copy link
Contributor

lukewalczak commented Oct 7, 2019

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
Copy link
Member Author

koke commented Oct 7, 2019

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
Copy link
Contributor

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
Copy link
Member Author

koke commented Oct 30, 2019

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
Copy link
Member Author

koke commented Oct 30, 2019

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

@lukewalczak
Copy link
Contributor

lukewalczak commented Oct 31, 2019

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
Copy link
Member Author

koke commented Nov 4, 2019

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:

  • 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.
  • Text should not shrink though (although it shouldn't go off-screen), and would force elements to wrap to the next row.

@iamthomasbishop
Copy link
Contributor

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 lukewalczak self-assigned this Nov 12, 2019
@lukewalczak
Copy link
Contributor

lukewalczak commented Nov 13, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • We could introduce a conflict of scroll directions (horizontal to "pan", vertical to dismiss)
  • Vertical seems a bit more scannable, especially as we add more blocks
  • I imagine as we scale this, we'll add something like tabs (for block categories), for which we may want to reserve the horizontal swipe gesture

@koke
Copy link
Member Author

koke commented Dec 20, 2019

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 WordPress/gutenberg#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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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

@lukewalczak
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants