[RNMobile] fix group appender behaviour

[jbinda]

Description

Refactor renderDefautBlockAppender() to unlock show InserterMenu in Group block (noticed issue after merge this one.

Please also refer to:
Related gutenberg-mobile PR

How has this been tested?

1. Add empty group block in initial-html

<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"></div></div>
<!-- /wp:group -->

2. Try to add block using appender
3. Select Group block and try to use group appender
4. Do the same on default initial-html content

I have also test it according to steps here

Test 1:

Test 2:

Test WPAndroid

Screenshots

Before:

After:

Types of changes

Small refactor to fix group appender behaviour

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. .

[jbinda]

@hypest I have provide the GIFs according to test you mentioned from original PR on demo app iOS/Android as well as in WPAndroid app

Appender seems to work as described there

[jbinda]

@iamthomasbishop

I talked with @hypest on Slack about this PR and we decided to ask you about the design. The issue was that I was unable to open InserterMenu after changes mentioned in PR description.

I provide simple solution that brings back ability to open InserterMenu with group appender but also will cause that user see the ADD BLOCK HERE separator above the appender. Is it something that breaks the current design ?

Our conclusion with @hypest was:

1. in my code we have the InserterMenu visible with Separator inside Group block
2. in @hypest version we do not have Separator in Group but also do not allow to open InserterMenu after press it
3. outside the Group block both version works the same
4. rendering BlockListAppender apparently plays a role here, which is still unclear which role it is or if it should play it.

[iamthomasbishop]

@jbinda It's a little hard to follow, but based on the "After" gif above, I think that's pretty close to what I'd expect. One change I'd make is to hide the inline appender (inside the group) while the block library bottom sheet is open, so just the "Add block here" indicator is shown in place of that.

Maybe I'm missing something, but that's my initial reaction.

[jbinda]

So I assume that is expected behaviour for Group block appender?

Now the technical aspects:
I have manage to allow see Inserter and hide the appender button after click to follow @thomasbishop guideline by adding check for isOpen in this renderToggle render prop inside ButtonBlickAppender like this:

Like me and @hypest conclude on Slack convo it seems to be "hacky" solution and it treated rather as "hotfix" if we do not found the root cause of described behaviour

According to that I have also investigate the flow behind BlockListAppender which is as below:

1. Inserter component is render in ButtonBlockAppender which passed render prop called renderToggle in which it renders the button with “+” icon and call onToggle which show and hide the InserterMenu with block options
2. in BlockListAppender we render CustomAppender or DefaultAppender. CustomAppender is render when we pass renderAppender to BlockListAppender. If this prop is not passed it render the default
3. in renderDefaultBlockAppender() method in BlockList we pass renderAppender from this.props. Tracking down the way from it comes I see that in Group block edit.native.js file we call InnerBlock component and pass there renderAppender in that way renderAppender={ isSelected && InnerBlocks.ButtonBlockAppender }. The flow of InnerBlock is that it renders BlockList with some props including renderAppender it receives (edited)

The next thing to be checked is value of willShowInsertionPoint under renderDefaultBlockAppender() which comes from shouldShowInsertionPointBefore()in BlockList component because it seems it also have impact of described behaviour.

[hypest]

Like me and @hypest conclude on Slack convo it seems to be "hacky" solution and it treated rather as "hotfix" if we do not found the root cause of described behaviour

Thanks for investigating deeper @jbinda ! Yeah, using isOpen couples the "+" button with the Inserter bottomsheet, which feels wrong. Let's continue working on this to find a better flag to depend the Group "+" button rendering on. Perhaps that flag could be something like the willShowInsertionPoint.

[iamthomasbishop]

So I assume that is expected behaviour for Group block appender?

@jbinda I think yes, it seems like the expected behavior.

[jbinda]

@hypest

I played again with the behaviour of this issue once again.

I came up with the idea to pass renderSeparator to BlockListAppender and conditionally render it under ButtonBlockAppender but I'm not sure we would like to go that way. Maybe we would like to change the structure where Inserter is rendered by Appender button ?

Anyway at this moment I have strong opinion that the solution this PR provide is currently the best option to allow show inserter when the group block do not have any children added.

The reason behind that is the current structure described in previous comment.

Do you have any thought on that since our previous discussion ?

[hypest]

Here's how I look at this: Looking at the ButtonBlockAppender component, I find it weird that it renders an Inserter component. I'd expect to render a "+" button. Maybe it makes sense for the web since the inserter is popped up right there by the button so the two are linked. On mobile it's not, rather, the inserter is rendered as a bottomsheet. Let's ignore that for a bit.

So as it happens, the Inserter renders the actual "+" button via the renderToggle() prop.

With the changes in this PR, the ButtonBlockAppender renders an Inserter, the Inserter renders a button via renderToggle and then renderToggle itself doesn't even render the button if the isOpen is true. I think this logic is quite complex to follow 🤔.

Instead, how about:

1. renderToggle always renders the button
2. Inserter decides to call renderToggle when closed and a renderAddBlockSeparator() function when open

Thoughts?

[dratwas]

1. renderToggle always renders the button
2. Inserter decides to call renderToggle when closed and a renderAddBlockSeparator() function when open

I totally agree with that flow :)

[hypest]

Can we consolidate this new component with https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list/insertion-point.native.js?

And similarly for the style definitions in https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/components/block-list/style.native.scss?

[dratwas]

I haven't realized that there is such a component. However, it seems like insertion-point.native.js is not used at all. I will move the AddBlockSeparator into it to be consistent with web names.

[hypest]

This looks good to me!

I've only left one comment about whether we can consolidate the older BlockInsertionPoint component with the newer AddBlockSeparator, but I think that could be done in a separate PR if needed.

[hypest]

LGTM!

Thanks for the work here @jbinda and @dratwas !

Feel free to merge, update the gutenberg-mobile PR with the merged commit and merge that one too.