Cross platform InspectorControls component

[pinarol]

We already have InspectorControls for mobile and web but it works differently.

Mobile is currently using InspectorControls as a slot to add buttons to the block toolbar, but web is using that slot to fill the block settings in the sidebar. What is more, mobile is showing/hiding the settings modal due to a flag in the state, so the whole thing acts a bit different than web. We should update InspectorControls mobile implementation to make it close to web as much as possible.

Here's a good description of this component for web: https://github.com/WordPress/gutenberg/tree/master/packages/block-editor/src/components/inspector-controls

And for mobile settings appear at the BottomSheet component:

We'll need to also add x-platform versions of these subcomponents used under InspectorControls
such as PanelBody, ToggleControl, SelectControl, RangeControl to be able to implement x-platform gallery block.

Good news is we have these ones covered:

ToggleControl > SwitchCell
SelectControl > PickerCell

But we are missing RangeControl so we'll also need to add support for that.

PanelBody : This one is a bit vague, we might not need to support it for mobile currently but only render a container View instead. We should decide on the way.

Let's do this in 2 iterations:

1. Refactoring the already existing usages on image, video blocks:
   gutenberg-mobile/gutenberg/packages/block-library/src/image/edit.native.js
   gutenberg-mobile/gutenberg/packages/block-library/src/video/edit.native.js

2. Giving support for the usage in Gallery block

To test

You can use example app for testing this.

yarn install
yarn start

yarn ios
yarn android

Make sure the current behaviour doesn't change, which is:

Add image/video blocks
Tap Settings button on the block's inner toolbar
Verify that block settings bottom sheet is displayed

[pinarol]

Keep in mind that this one has lower priority than MediaUpload and MediaPlaceholder

[pinarol]

hi @jbinda 👋 Do you have any initial thoughts about how to approach the problem? I encourage you to share those early on or share some draft PRs to discuss on it.

[jbinda]

Hi @pinarol I have split my work to 3 steps instead of 2 you have mentioned in the issue

1st step was to cover RangeCell (slider)
2 refactor InspectorControls component
3 give support in gallery block

I will try to post PR with my concept on point 1 and 2 by the end of the day

[jbinda]

@pinarol
I did PR to present my solution. Still need to apply fix to pass the tests but I would like you to do some initial review on the approach.

In the meantime I would like to ask about below concerns:

1. Is the BottomSheet component should be the only one to show options after press settings button or you would like to have flexible way to pass other container component as well
2. I've moved showSetting state prop from edit.native.js of the block to its parent component <BlockListBlock> in block native.js. The goal here was to achieve consistent InspectorControls api with web version. Perhaps it should be also moved e.g. inside InspectorControls src files

Important thing here is that on the web the block settings option button is always visible on the top bar. On mobile the button is visible only when the block is selected - you would like to keep that behaviour or move settings button e.g to <BlockControls> ?

Please let me know what do you think about above and about my comments in raised PRs

[pinarol]

Hi @jbinda 👋

Is the BottomSheet component should be the only one to show options after press settings button or you would like to have flexible way to pass other container component as well

I don't expect pressing the settings button would do something different other than opening the settings BottomSheet.

But we might want to add different buttons near the settings button in the future so I'd keep an open slot for that.

I've moved showSetting state prop from edit.native.js of the block to its parent component in block native.js. The goal here was to achieve consistent InspectorControls api with web version. Perhaps it should be also moved e.g. inside InspectorControls src files

I wonder how others think about this but I feel like we can extract a new component as SettingsToolbarButton and keep showSettings there. But I don't have strong opinions.

Important thing here is that on the web the block settings option button is always visible on the top bar. On mobile the button is visible only when the block is selected - you would like to keep that behavior or move settings button e.g to ?

For now let's keep the same behavior for mobile.

[jbinda]

Hi @pinarol

Thanks for response. I will prepare changes according to comments in PR and let you know to take a look once again

[jbinda]

Hi @pinarol

I did update in InspectorControls PR. If you have time to take a look it would be great!

I will also provide GIF to present how the settings button works.

Another thing is I still need to deal with test case in gutenberg because on CI some of them are failing. If you have any advice about failed test it's more that welcome.

[jbinda]

Do you have any preferences if I would like to share you a sample GIF as en example (100mb +) or I can use e.g giffie (I also play with code a little bit there)?

[pinarol]

Do you have any preferences if I would like to share you a sample GIF as en example (100mb +) or I can use e.g giffie (I also play with code a little bit there)?

We usually create the gif in our local and upload directly here, I personally use Licecap but it is up to you.

[pinarol]

Another thing is I still need to deal with test case in gutenberg because on CI some of them are failing. If you have any advice about failed test it's more that welcome.

For the CI, it needs to be investigated case by case. As a first step I recommend getting this PR merged and updating your branch from develop. It's detected that there was a redundant line that caused some intermittent behavior on CI tests.

[jbinda]

Ok I'll try to pull this changes and check if that helps

[jbinda]

Hey, please check below GIF to see how the InspectorControls works on mobile with code Ive provided:

GIF or mp4

As you can see there is an option to add an extra settings button add pin custom action as well. The thing that might be also needed is to distinguish what should be render inside InspectorControls when there are more than one settings button. I believe that we can add this logic in the next step in the future.

Please let me know what do you think.

[pinarol]

As you can see there is an option to add an extra settings button add pin custom action as well

Actually we won't have another settings button, we need only 1. We might have other buttons there in the future but they would have totally different purposes. We shouldn't assume those will be opening BottomSheet too. At the moment we don't have to worry about those extra buttons.