[RNMobile] refactor InspectorControls

[jbinda]

Description

PR is connected with #1300 in gutenberg-mobile.

Please also refer to Related gutenberg-mobile PR

It presents:

1. PanelBody mobile implementation (currently simple View component)
2. Refactor of InspectorControls that allows to use it in pretty same manner as on web
3. Refactor in edit.native.js in image and video block
4. Update SettingsButton in BlockMobileToolbar behaviour to allow show/hide depending if there is any settings available to current block

How has this been tested?

Currently only manual test was provided to check if feature listed in the #1300 works as expected.
Also still need to fix some failing unit test but I will be cover soon.

Screenshots

Please see below screenshot. It presents how to render InspectorControls inside Image block. On left side there is usage on Web. On the right side is the usage on mobile.
Note here that I temporally left BottomSheet.Cell to render buttons inside BottomSheet - the markup will be also refactor in separate PR to be consistent with web markup.

Types of changes

It should not have impact on usage on web but provides changes in usage on mobile (see comments in changes section in this PR)

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.

[pinarol]

hey @jbinda Thanks for the PR 🎉 I tried to address all the main things but let me know if I missed something

[pinarol]

I am also inviting @koke and @marecar3 as reviewers as InspectorControls is one of the most commonly used components.

[pinarol]

Let me also link this comment here in case you have different opinions @koke @marecar3 please go ahead and respond here: wordpress-mobile/gutenberg-mobile#1300 (comment)

[koke]

I agree with your second thought. I don't think we'll need the slot for now, and if/when we do it should probably be called something else.

[pinarol]

I didn't understand the need for putting BottomSheet component inside slot-fill. slot-fill is a multi purpose pattern, would it be possible if we don't touch it at all for this particular change?

[koke]

I agree, I don't see the need to change slot-fill for this. I'd recommend looking at how this works on the web:

• @wordpress/edit-post has a Layout component that includes a Sidebar.Slot
• I believe that the sidebar visibility is controlled via the core/edit-post store with the isEditorSidebarOpened selector. The button to open the sidebar would dispatch the openGeneralSidebar action.
• It also includes a SettingsSidebar that fills that slot with many things, including BlockInspector.
• This BlockInspector is where InspectorControls.Slot is placed

For mobile, we use a modal BottomSheet instead of the Sidebar (although I imagine iPads could show a sidebar in the future), and the button to toggle it is in the block toolbar instead of a global editor toolbar.

The UI is a little different but I think the existing web architecture should still fit the native UI.

[jbinda]

@koke, @pinarol thank you for review.

I followed with the remarks you have posted and implement changes as below:

1. I have moved BottomSheet under Layout component and provide BottomSheetSettings component to make it more similar to web (which use slot-fill logic to render Sidebar and it's content)
2. I use logic from web to show/hide the settings BottomSheet container
3. I have created BlockSettingsButton and use slot-fill logic because I need to conditionally show/hide it depending if given block have some settings wrapped in InspectorControls
   4 I created native implementation for InspectorControls which will render the same as for web plus SettingsButton

In current implementation I was able to remove all changes in slot-fill logic made before (only InspectorControls for mobile is affected) and also the behaviour is the same as I recorder before. You can noticed that after you delete the children of InspectorControls in edit.native.js for image block the settings button disappears.

[Tug]

Nice refactor here 👍
I like the fact that we removed some of the internal state of the ImageEdit component showSettings and moved it to the store as editorSidebarOpened.
Not sure why we would want a Slot for BlockSettingsButton though.

[Tug]

I don't think it's necessary to make the settings button a SlotFill. I don't expect another button to make sense being displayed alongside it.

[jbinda]

It's because we render the settings button Fill component inside Inspector Controls when it has any children. Using it in that way we can automatically show/hide setting button depending if block has some settings wrappen insideInspectorControl component inside edit.native.js file.

At this moment it was my best idea to make the API on mobile usage as much as possible to web.

Another benefit of that is after small refactor we can use the same mechanism to add another buttons in the future

[koke]

Using it in that way we can automatically show/hide setting button depending if block has some settings wrapped inside InspectorControl component

There was something bugging me about this and I realized that the behavior is different on the web, as the settings button is always present, because the sidebar contains other things as well. After chatting with @iamthomasbishop, we think it would be interesting to have the block settings button always available on every block. I don't think we need to block this PR on that, but it would be good to consider after merging this.

On the web, even if you ignore the document options, block settings always shows the block icon, title, and description, and most blocks have the advanced css field:

The blocks currently available on mobile don't implement those InspectorControls yet in their native variants, but once they do, I'd change the behaviour to match the web.

[iamthomasbishop]

Agreed w/ @koke – if we add the additional css class input on every block, we can safely add (and should add) the cog/settings trigger to every block. There has been some user confusion around not seeing settings for every block type, and while we don't have all the settings for a bit, it'll at least relieve the confusion around the entry point.

[jbinda]

@koke - right! that was the main difference. On web there are others element that has a slot inside Settings Sidebar. On mobile we have less space to show them all at once. Stacked bottom sheets seems to be good solution

@iamthomasbishop - as you mentioned in current implementation not every block has a settings button. So the fastest way to show it in every block is to take care about AdvancedInspectorControl mobile implementation right ? (this is the component which renders Additional CSS classes)

Summarizing: If it's ok if we merge this PR and then take care of AdvancedInspectorControls to finally refactor settings button show/hide logic ? Or we should wait with merging ?

[pinarol]

Summarizing: If it's ok if we merge this PR and then take care of AdvancedInspectorControls to finally refactor settings button show/hide logic ? Or we should wait with merging ?

I don't think we should block this PR for that, we can handle it separately.

[jbinda]

Ok, so I resolve conflicts and it's ready to merge

[iamthomasbishop]

Agreed w/ @pinarol – this isn't a blocker.

So the fastest way to show it in every block is to take care about AdvancedInspectorControl mobile implementation right ? (this is the component which renders Additional CSS classes)

Yes, essentially just adding the additional css classes input should in theory take care of the problem, bc every block (afaik) has that field.

[pinarol]

Could you update the branch and resolve the conflicts? @jbinda

[pinarol]

Opened an issue to handle advanced settings separately.

[jbinda]

Sure !

[jbinda]

@pinarol conflicts are solved and CI passed :)

[pinarol]

@jbinda could you update target branch as master?
@koke could you give a quick re-review afterwards and then we can merge it right?

[jbinda]

@pinarol already in-progress :)

[koke]

Yes, code looks good. I was planning to do some final testing today before merging

[jbinda]

I just wonder if we shouldn't first merge rmobile/master to master ?

[pinarol]

It is already done by #17228

[jbinda]

Ok, thanks !

[koke]

Looking good. I saw one failing test, so I restarted the build on Travis to see if it was a glitch. Ready to merge when green IMO

[jbinda]

ok, thanks for restart the test.

It's something with unit test. Locally it also fails

[jbinda]

And there is also failing test on Android on develop branch in gutenberg-mobile. Try investigate what's the cause of failure on this tests.

[jbinda]

@koke @pinarol - it seems that it really was a glitch. I didn't found anything that can break test and after rerun CI it passed in both repo so I think it's ready to merge.

[pinarol]

LGTM 🎉

[jbinda]

great ! Thanks