[RNMobile] Adjust bottom-sheet #18574

lukewalczak · 2019-11-18T15:02:44Z

Description

Ref to gb-mobile PR: wordpress-mobile/gutenberg-mobile#1837

That PR is fixing the issue with wrong layout within inserter: wordpress-mobile/gutenberg-mobile#1404

That PR is fixing: wordpress-mobile/WordPress-iOS#13304.

NOTE: At the same point, it's disabling swipe-to-close option on Android ( swipe / drag is still available in the top most area - near drag indicator).

How has this been tested?

Run the app on a smaller device or on the regular in horizontal mode
User should be able to scroll through all the blocks / add them
Please check also other bottom-sheets in the app e.g: Image settings

Screenshots

ios	android

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

pinarol · 2020-01-30T10:56:05Z

cc @iamthomasbishop Could you check the screen captures in this PR?

pinarol · 2020-01-30T11:08:25Z

cc @koke could you also verify this is solving the problems you mentioned in the issue?

dratwas · 2020-01-30T12:09:35Z

Hi! It is only a temporary solution since in the future we will be able to have swipe with the scroll on Android as well. We just need to wait for that fix to be merged software-mansion/react-native-gesture-handler#937.

Some context:
I was trying to achieve the perfect solution with a scroll view and swipe but failed because of an issue with react-native-gesture-handler. I used react-native-reanimated-bottom-sheet that supports all we need (ScrollView inside, swipe to hide, keyboard aware view) but - It is rendered inside our RN root view that doesn't cover the whole screen (we have a native navbar that is above) and it looks like that:

We need to render it inside react-native Modal component that creates a separate native view that covers the whole screen. Unfortunately, react-native-gesture-handler doesn't work on Android in Modal software-mansion/react-native-gesture-handler#139 . Fortunately, @osdnk already created a fix for that 🎉 software-mansion/react-native-gesture-handler#937 I tested it and it seems to work. We need to wait for reviews, merge, and release and we will be able to implement the Bottom Sheet that works as we wanted. In addition, we will change the old PanResponder that is controlled by the JS responder system to a more performant one.

Technically we are able to make it with react-native-modal that we use at this moment but it works really really bad because when it recognizes that the touch should be handled by Scroll View it calls scrollView.scrollTo() to scroll instead of swipe. However, it doesn't have any easing etc, so the feeling when scrolling is terrible.

pinarol · 2020-01-30T12:39:51Z

Thank you @dratwas for those great news and all your effort on that!

To sum up, until the time we have that perfect solution we'll need to solve the issue about block picker not fitting into small screens anymore. So I believe this PR brings a good short term solution to that.

lukewalczak · 2020-01-30T17:06:15Z

packages/components/src/mobile/bottom-sheet/index.native.js

+						scrollEventThrottle={ 16 }
+						style={ { maxHeight } }
+						contentContainerStyle={ [ styles.content, contentStyle ] }
+					>
 						{ this.props.children }


One thing which is worrying me ( and I'm not proud of it :( ) is placing a FlatList within a ScrollView. Generally, it shouldn't have any performance issues since scrolling is disabled in a FlatList. There is no issue with rewriting it, but we're going to lose nice features such as ItemSeparatorComponent or numColumns. Please let me know wdyt.

lukewalczak · 2020-01-30T17:30:53Z

One more thing which I think it's worth mentioning is these changes affect (fixes) other bottom-sheets in the application, eg: Image settings. This is how it looks like on iPhone 6 with the largest font:

before	after

As you can notice in the screenshot before the change, there is not too much free space and keep in mind that e.g Button block will have more settings inside.

iamthomasbishop · 2020-01-30T18:09:33Z

This is much better than what exists now so I agree w/ @pinarol we should ship these improvements and iterate. With that said, I have a few bits of feedback:

The sizing/spacing feels a bit off on the top drag handle. IIRC, the drag handle area should be ~16px and the scroll area should be flush up against that "container". Example EDIT: The scroll container itself is flush up against the drag handle container, but there is 8px of padding-top on the scroll container. So visually, it is 24px from the top — 16px of that is the drag handle area, 8px is additional padding-top on the scroll container. Another visual example here.
It looks like something is off about the bottom safe area around the home indicator. Sometimes the contents of the sheet scroll directly beneath the home indicator (which is what I would expect) and sometimes they do not. Is there any way we can remove the background fill from the safe area that covers the sheet contents? Examples: Expected, not covering the contents vs. Covering

NOTE: At the same point, it's disabling swipe-to-close option on Android ( swipe / drag is still available in the top most area - near drag indicator).

Can the user tap the scrim to dismiss the sheet? I would hope so. Even if they can, because we won't provide swipe-to-dismiss on Android, we might want to target that for a shorter max-height on Android so that the scrim and drag handle are slightly more reachable while still showing ~2.5 rows of blocks — maybe ~250-260px tall so that it looks something like this?

lukewalczak · 2020-01-31T10:46:22Z

UPDATES:

Horizontal:

top	during scroll	bottom

Vertical:

top	during scroll	bottom

lukewalczak · 2020-02-03T13:08:48Z

UPDATES

Fixed bottom area inset screenshot
Fixed landscape mode screenshot

Q&A

On Android I still see a two columns inserter:

This is kind of expected behavior, especially on devices with an 18:9 (or 18,5:9) aspect ratio and enabled screen zoom on max.

Number of columns is calculated based on Dimensions.get( 'window' ).width / itemWidth, where itemWidth is fixed and value is 120. The problem here is the width from Dimensions which is changing accordingly to screen zoom (the width is decreasing while screen zoom is increasing). That's why e.g on Samsung S8 on neutral zoom we'll see 3 columns (width is ~411), but on max zoom we'll see 2 columns (width is ~320)

neutral zoom	max zoom

There seems to be bouncing even when the content is smaller than the maximum height

Tbh, I was not able to reproduce it before refactor and after as well. Please check it one more time, if you will face that issue again, please send me some repro steps along with device configuration / specs 🙏

iamthomasbishop · 2020-02-03T16:56:19Z

This is kind of expected behavior, especially on devices with an 18:9 (or 18,5:9) aspect ratio and enabled screen zoom on max.

@lukewalczak I think even with "large" display zoom, we should ensure there are 3 columns per row. If we need to adjust the spacing around grid items, widths, or font-sizes, then let's do that.

iamthomasbishop · 2020-02-03T16:57:43Z

Also, the other screenshots above (bottom sheet inset and landscape mode) look great 👍

lukewalczak · 2020-02-04T10:41:53Z

UPDATES

Adjusted item width for a zoomed screen on Android:

regular	zoomed

etoledom · 2020-02-04T12:19:13Z

UPDATES

Adjusted item width for a zoomed screen on Android:

Looking great on Android now! 🎉
Thank you for the updates.

@etoledom It's hard to tell from the gif above, but does that mean you're not able to swipe to dismiss the sheet?

@iamthomasbishop no, it just tries to scroll the content.

Tbh, I was not able to reproduce it before refactor and after as well. Please check it one more time, if you will face that issue again, please send me some repro steps along with device configuration / specs 🙏

@lukewalczak you are right, it's not the normal behaviour, but a corner case. That's probably why you couldn't reproduce it. I was able to reproduce it on an iPhone XS with these steps:

With the device on Landscape, open an image block's settings sheet.
Start written a link or an Alt text.
Notice that now the sheet is smaller than the available space, so it becomes scrollable (that's good ✅ ). Also notice that the top went out of the screen (not so good, but not alarming).
Scroll all the way until the Clear All Settings button is visible.
With the Clear button visible and the keyboard still present, rotate the device to Portrait mode.
Now the Bottom Sheet bounces being smaller than half screen.
You can close it and open the same settings, and the problem persist without the need of the previous steps.

This being a corner case, I don't consider it a blocker for this PR, but it would be great to fix it if it's not too complex.

EDIT: Adding a video showing the steps to reproduce the issue: https://cloudup.com/cBxQc0qynbx

Let me know!

lukewalczak · 2020-02-06T13:51:57Z

UPDATES

BottomSheet height fixed along with redundant scrolling

etoledom

The remaining issues seem to be fixed. Looking great to me! 🎉

Super job @lukewalczak ! I'm so glad we will finally have a scrolling bottom sheet

packages/block-editor/src/components/inserter/menu.native.js

packages/block-library/src/button/color-background.native.js

Initial work on iOS Inserter

f8fdeaf

lukewalczak changed the title ~~Initial work on iOS Inserter~~ [RNMobile] Initial work on iOS Inserter Nov 18, 2019

lukewalczak mentioned this pull request Dec 19, 2019

Inserter shows wrong layout with Display Zoom enabled wordpress-mobile/gutenberg-mobile#1404

Closed

lukewalczak added 3 commits December 19, 2019 21:54

Merge branch 'master' into rnmobile/inserter

b190a91

Merge branch 'master' into rnmobile/inserter

a020ec0

Disable swiping-to-close on Android

7ef43f6

lukewalczak marked this pull request as ready for review January 30, 2020 08:56

lukewalczak changed the title ~~[RNMobile] Initial work on iOS Inserter~~ [RNMobile] Adjust bottom-sheet Jan 30, 2020

pinarol requested a review from etoledom January 30, 2020 10:54

lukewalczak mentioned this pull request Jan 30, 2020

Correct layout within bottom-sheet [workaround] wordpress-mobile/gutenberg-mobile#1837

Merged

2 tasks

lukewalczak commented Jan 30, 2020

View reviewed changes

Merge branch 'master' into rnmobile/inserter

54196b2

lukewalczak force-pushed the rnmobile/inserter branch from 371174d to 54196b2 Compare January 30, 2020 17:08

lukewalczak added 2 commits January 31, 2020 10:12

Add listener for device rotation and small styling changes

c2b05b0

Merge branch 'master' into rnmobile/inserter

d311517

lukewalczak force-pushed the rnmobile/inserter branch from ba954ef to d311517 Compare January 31, 2020 10:28

lukewalczak force-pushed the rnmobile/inserter branch from 786c6df to d311517 Compare January 31, 2020 12:48

lukewalczak added 2 commits January 31, 2020 13:54

Merge branch 'master' into rnmobile/inserter

e897e67

Small cleanup

edfc88c

lukewalczak force-pushed the rnmobile/inserter branch from f84965b to edfc88c Compare January 31, 2020 13:10

More refactor

84fa9f8

etoledom assigned lukewalczak Jan 31, 2020

etoledom added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jan 31, 2020

lukewalczak force-pushed the rnmobile/inserter branch from 4190f55 to 6292cfb Compare February 3, 2020 13:47

Merge branch 'master' into rnmobile/inserter

d58567c

lukewalczak force-pushed the rnmobile/inserter branch from 6292cfb to d58567c Compare February 3, 2020 13:48

Adjust itemWidth for zoomed screen on Android

65c54d2

Set accessiblity false

359f566

lukewalczak force-pushed the rnmobile/inserter branch from 04f1c6f to e70b44b Compare February 5, 2020 09:40

Merge branch 'master' into rnmobile/inserter

01c18d4

lukewalczak force-pushed the rnmobile/inserter branch from e70b44b to 01c18d4 Compare February 5, 2020 09:48

lukewalczak added 3 commits February 5, 2020 16:19

Add disableScrollViewPanResponder

89684ee

Adjust BottomSheet height when a keyboard is opened

1efdd26

Correct removing keyboard listeners

60daed9

etoledom approved these changes Feb 10, 2020

View reviewed changes

lukewalczak added 4 commits February 11, 2020 11:31

Merge branch 'master' into rnmobile/inserter

45279b5

Code clean up

b282411

Revert change

4ed2acb

Revert pushed file by mistake

fbb6a9f

dratwas reviewed Feb 11, 2020

View reviewed changes

lukewalczak added 2 commits February 11, 2020 14:47

Code refactor after CR

b5a1c12

Remove dimensions listener on unmount

fe5ec26

lukewalczak merged commit d67ce67 into master Feb 12, 2020

lukewalczak deleted the rnmobile/inserter branch February 12, 2020 11:17

github-actions bot added this to the Gutenberg 7.6 milestone Feb 12, 2020

dratwas mentioned this pull request Feb 12, 2020

BottomSheet with scroll view inside - ideal version wordpress-mobile/gutenberg-mobile#1884

Closed

lukewalczak mentioned this pull request Feb 14, 2020

Update release notes wordpress-mobile/gutenberg-mobile#1908

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Adjust bottom-sheet #18574

[RNMobile] Adjust bottom-sheet #18574

lukewalczak commented Nov 18, 2019 •

edited

Loading

pinarol commented Jan 30, 2020

pinarol commented Jan 30, 2020

dratwas commented Jan 30, 2020 •

edited by lukewalczak

Loading

pinarol commented Jan 30, 2020 •

edited

Loading

lukewalczak Jan 30, 2020

lukewalczak commented Jan 30, 2020

iamthomasbishop commented Jan 30, 2020 •

edited

Loading

lukewalczak commented Jan 31, 2020 •

edited

Loading

lukewalczak commented Feb 3, 2020

iamthomasbishop commented Feb 3, 2020

iamthomasbishop commented Feb 3, 2020

lukewalczak commented Feb 4, 2020

etoledom commented Feb 4, 2020 •

edited

Loading

lukewalczak commented Feb 6, 2020

etoledom left a comment

[RNMobile] Adjust bottom-sheet #18574

[RNMobile] Adjust bottom-sheet #18574

Conversation

lukewalczak commented Nov 18, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

pinarol commented Jan 30, 2020

pinarol commented Jan 30, 2020

dratwas commented Jan 30, 2020 • edited by lukewalczak Loading

pinarol commented Jan 30, 2020 • edited Loading

lukewalczak Jan 30, 2020

Choose a reason for hiding this comment

lukewalczak commented Jan 30, 2020

iamthomasbishop commented Jan 30, 2020 • edited Loading

lukewalczak commented Jan 31, 2020 • edited Loading

lukewalczak commented Feb 3, 2020

iamthomasbishop commented Feb 3, 2020

iamthomasbishop commented Feb 3, 2020

lukewalczak commented Feb 4, 2020

etoledom commented Feb 4, 2020 • edited Loading

lukewalczak commented Feb 6, 2020

etoledom left a comment

Choose a reason for hiding this comment

lukewalczak commented Nov 18, 2019 •

edited

Loading

dratwas commented Jan 30, 2020 •

edited by lukewalczak

Loading

pinarol commented Jan 30, 2020 •

edited

Loading

iamthomasbishop commented Jan 30, 2020 •

edited

Loading

lukewalczak commented Jan 31, 2020 •

edited

Loading

etoledom commented Feb 4, 2020 •

edited

Loading