[RNMobile] Adjust vertical margins in InnerBlock

[jbinda]

Description

PR provides additional login to adjust vertical margins when navigate through InnerBlock hierarchy.

@iamthomasbishop,
With this changes navigate inside InnerBlock becomes smoother. The spaces between block is kept with the same value. Please taka a look on presented GIFs

Please also refer to:
Issue
Related gutenberg-mobile PR

How has this been tested?

1. Add below code in initial-html. It provides some nested InnerBlock structure

/**
 * @format
 * @flow
 */

export default `
<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:heading -->
<h2>Test Heading</h2>
<!-- /wp:heading --></div></div>
<!-- /wp:group -->

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

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

<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group --></div></div>
<!-- /wp:group --></div></div>
<!-- /wp:group --></div></div>
<!-- /wp:group -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image -->

<!-- wp:image -->
<figure class="wp-block-image"><img alt=""/></figure>
<!-- /wp:image --></div></div>
<!-- /wp:group -->


`;

2. Try to play with nested Group block and see that vertical margins between block should be kept with same value
3. Notice that there is no margins multiplication anymore when nest multiple empty Group Blocks
4. Feel free to complicate this structor further more and observe the vertical margin behaviour

Screenshots

Types of changes

Bug fix / refactor

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

[iamthomasbishop]

This looks great, well done!

I have a separate question: I noticed in some cases (particularly noted on the iOS screen cast above), multiple levels of children (dashed outlines) are shown. Example:

I believe only the first child level should be visibly outlined.

[jbinda]

@iamthomasbishop
The example you gave it's slight different than you described with below words:

multiple levels of children (dashed outlines) are shown

It is default AppenderButton behaviour which instead of (+) Icon with solid border renders dashed borders as placeholder.

In e.g. Image Block placeholder makes nice look with dashed border applied and light grey background. Comparing group placeholder to that you can be confused a little bit. By on the other hand it make sense and it's consistent with behaviour in the root list of blocks. Please see below GIF. There are two Groups in the root list. The first one have Heading and empty Group block inside. Second one is empty and do not have any nested blocks. As I mentioned above the double dashed border is because with InnerBlock styling logic we apply dashed border to all children of selected Group. We don't do the same at the RootList level.

I believe only the first child level should be visibly outlined.

That's the way how it works but the effect you noticed is caused by reason I explained above.

I hope all above make sense.

The possible solution that I have in my mind are:

1. Conditionally turn off applying the dashed border for child group that is empty (then the dashed border will comes from the placeholder)
2. Conditionally turn off dashed border in group placeholder when the group is inside InnerBlock
3. Instead of render placeholder render the AppenderButton with some dimming of different style that point it's state to inactive
4. Add placeholder that is more similar to other blocks placeholder

Keep in mind the first two solution brings more complexity to bordering style in the navigation-dow logic because depending of placement in hierarchy we should modify the margins as well to align the content

Please let me know what do you think and how we should approach this according to open Group block to production.

[jbinda]

I'm not sure it's a blocker here.

However I also think that it might be worth to add some animation in Breadcrums on FloatingToolbar when selection inside InnerBlock changes. I thought initially about little dim out/ dim in the name/icon of blocks (or similar animation that we have after click "Go Up" button).

Why ?

E.g. because when you nest multiple empty Group block you can loose orientation after press (the UI doesn't change) but in fact the selection does and you go deeper and deeper after each press. In other cases it will bring some nice-looking effect. Please take a look on below GIF:

The structure of Blocks is presented here (nested 8 empty Group blocks):

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

Please observe that you do not see any effect when you select 3rd to 7th (yeah I'm pressing the block repeatedly). The UI changes only when you select the last one (because the placeholder is replaced with AppenderButton) and when you select first two (apply bordering style, breadcrumbs show the current hierarchy, navigation arrows is disabled).

WDYT ?

[koke]

If I understand this correctly, this is trying to add a margin to every sibling except the last one, right?

This expression seems more complicated than it needs, if I understand each variable correctly. I see three possible cases regarding the number of children in the parent:

• There's one block (parentCount < 2). Then this must be the last (and first) block, so ! isLastBlock === false and the margin shouldn't apply.
• There are two blocks (parentCount == 2). We have already set a condition to not add a margin on the last block, so the isFirstBlock is only relevant for the other case, which will always be true: if I'm not the last of 2, I must be the first.
• There are more than two blocks (parentCount > 2). Similarly, we've ruled out the case where we are the last block, and we want to add the margin for every other block.

So, if I'm not missing anything, the same logic should work with this simpler condition:

Suggested change

	const shouldApplyVerticalMarginStyle = ! isLastBlock && ( ( isFirstBlock && parentCount === 2 ) || parentCount > 2 );
	const shouldApplyVerticalMarginStyle = ! isLastBlock;

[jbinda]

I have check it and it's fine but after we modify isLastBlock as below:

In previous implementation it tries to check the order base of current clientId InnerBlocks count. Which isn't proper because we need to check the order in parent InnerBlock list.

I have checked and it seems that this flag isn't use anywhere so probably we can also prevent to pass it to component props

[jbinda]

I have also simplify passing the prop in latest commit

[koke]

Conditionally turn off dashed border in group placeholder when the group is inside InnerBlock

I think that one makes the most sense. The placeholder border "conflicts" with the nested blocks child border, and should only be used in root-level groups.

[jbinda]

I think that one makes the most sense. The placeholder border "conflicts" with the nested blocks child border, and should only be used in root-level groups.

Logically it works fine because if the group is empty it has a dashed placeholder. Considering UX I think it also looks fine.

However I do not have strong opinion on that and we can try implement above solution.

[koke]

It's a bit confusing navigating these many nested groups, but I think this is behaving correctly 😅

[jbinda]

It's a bit confusing navigating these many nested groups, but I think this is behaving correctly 😅

I think we also bring some refactor soon to improve UX according to this comment according to this convo

One more question. Shall we remove the devOnly flag with this PR or create a new one ?

[koke]

One more question. Shall we remove the devOnly flag with this PR or create a new one ?

Ah, yes please, let's move that change to a different PR