[RNMobile] Styling fixes after navigation merge

[lukewalczak]

Description

That PR is fixing the issues mentioned in testing release after navigation feature merge

Ref to gb-mobile: wordpress-mobile/gutenberg-mobile#1682

How has this been tested?

• As discussed earlier, there is an issue w/ the spacing of the Inline Toolbar.

I fixed vertical and horizontal padding in BlockMobileToolbar

not nested	nested	unsupported

• The margins/padding on the Columns block look a bit broken (screenshot). Unsupported, I know, but I tried adding one on web

• Can we hide the Floating Toolbar on unsupported blocks like the Columns block, so that they don't say "unrecognized"?

Fixed both of them!

• It looks like there is extra padding applied on the Media & Text block now?

I was not able to reproduce it and it looks properly in my opinion

nested	not nested

• I know we had discussed the Appender spacing, but I think we might want to align to the same visual keyline as the child block borders

Fixed!

group with children	empty group

• Is it possible to remove the opacity-change effect when the breadcrumbs are tapped? Because nothing happens when I tap it (which is fine until we have the doc navigator), it might feel like a bug to users.

I disabled that touchable since there is an empty onPress function

• When I add any type of RichText block within an empty Group, the input isn't focused right away – this felt odd, and it doesn't happen on a non-empty Group block.

Honestly, I would like to leave it for now, since I don't remember what was the cause of jumpiness in RichTexts and back to that once Jakub will be available.

Screenshots

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

[lukewalczak]

Can we hide the Floating Toolbar on unsupported blocks like the Columns block, so that they don't say "unrecognized"?

I've realized right now, that your suggestion is partly incorrect. What I mean is I agree that we can hide a toolbar when an unsupported block isn't nested, but in nested case, we should find other solution such as:

• hide only breadcrumbs, but keep an arrow to move to the parent
• Unrecognized block change for Unsupported block

@iamthomasbishop Please let me know what do you think about it

[iamthomasbishop]

Thanks for the quick fixes, everything is looking great here! Glad to see the Media & Text block is displaying properly here, I'm not sure why but when I tested, I was seeing the Media & Text block still side-by-side (not stacked as I'm seeing in your screenshots above, which is perfect).

I have 1 question, which is not an issue we need to solve here: in the screenshot above where you have the Media & Text block (parent) selected, is the media part of that not shown as a child because of the default selection logic – meaning it's "auto" selected by default?

[iamthomasbishop]

I've realized right now, that your suggestion is partly incorrect. What I mean is I agree that we can hide a toolbar when an unsupported block isn't nested, but in nested case, we should find other solution such as:

Now that I look at it again, I wonder if it'd be possible to just make sure the Unrecognized label says the name of the selected block (in my example, Columns). Is there a challenge in getting that label to match the selected block?

[lukewalczak]

I have 1 question, which is not an issue we need to solve here: in the screenshot above where you have the Media & Text block (parent) selected, is the media part of that not shown as a child because of the default selection logic – meaning it's "auto" selected by default?

Sorry, but I don't understand what you meant 😢

Now that I look at it again, I wonder if it'd be possible to just make sure the Unrecognized label says the name of the selected block (in my example, Columns). Is there a challenge in getting that label to match the selected block?

I'll see what I can do! If I succeed then should I change the label in both cases or only in one (nested)? Also, what should I do in the scenario where getting the name will be too complicated?

[iamthomasbishop]

Sorry, but I don't understand what you meant

Apologies, my comment wasn't super clear. I meant there is no "child" outline style on media part of the Media & Text block. I was assuming this was related to our current selection logic, where the media part is auto-selected initially.

I'll see what I can do! If I succeed then should I change the label in both cases or only in one (nested)?

My thought is any time the floating toolbar is shown, the breadcrumb should say the name of the block that is selected. "Unrecognized" seems a bit weird to me.

Also, what should I do in the scenario where getting the name will be too complicated?

If impossible, I would make the label Unsupported.

[lukewalczak]

@iamthomasbishop I've checked the possibilities to provide the original block name along with an icon will be complex in relation to benefit, that's why I've prepared a solution with Unsupported label, which is better than Unrecognized Block

nested	not nested

Also, after pulling fresh changes I've noticed that items within Media&Text block are aligned one above the other on horizontal mode. You can check it here

[iamthomasbishop]

I've checked the possibilities to provide the original block name along with an icon will be complex in relation to benefit, that's why I've prepared a solution with Unsupported label, which is better than Unrecognized Block

This is definitely better. Is it also a similar difficulty to display the selected block icon (Columns icon in your example above) in the breadcrumbs instead of the generic block icon? Not the end of the world but it would be even nicer 😄

Also, after pulling fresh changes I've noticed that items within Media&Text block are aligned one above the other on horizontal mode. You can check it here

👍

[pinarol]

This looks good to me overall, let's wait for ✅ from @koke as well.

I spotted a weird anomaly during testing this but it turns out it has nothing to do with this PR.

[koke]

I think I'd rather avoid using negative margins here. It seems that we should instead wrap the block content in another view and add the padding to that instead.

[lukewalczak]

Actually I was afraid of negative margins as well, but decided to go with them because touching other styles were more dangerous for me.

[koke]

I'm afraid unregistered/unsupported blocks might be a bigger deal than it seems and that we didn't anticipate in the previous PR. The problem is that they are rendering as a simple core/missing, but all the inner blocks navigation and styling logic is actually looking at the real block data that's unsupported. So even with this PR, if you nest a columns block inside a group, you get the wrong margins:

One approach that initially seems to be working is to change the hasChildren check instead:

const hasChildren = ! isUnregisteredBlock && !! getBlockCount( clientId );

[koke]

Unrelated to this I think, but the appender on an empty group isn't working. I thought I saw a PR fixing that a while back.

I know we had discussed the Appender spacing, but I think we might want to align to the same visual keyline as the child block borders

Shouldn't the empty group also get the same spacing?

[lukewalczak]

Unrelated to this I think, but the appender on an empty group isn't working. I thought I saw a PR fixing that a while back.

I know we had discussed the Appender spacing, but I think we might want to align to the same visual keyline as the child block borders

Shouldn't the empty group also get the same spacing?

I think that empty group appender has a proper spacing, but please @iamthomasbishop let us know if this is correct.

empty group appender	filled group appender

[lukewalczak]

@koke Thanks for feedback and suggestions.
I've refactored:

• Change Unrecognized Block in favour of Unsupported in missing block
• Fix padding for nested Unsupported block, this is how it looks right now:

Testing HTML

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

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

<!-- wp:paragraph -->
<p>Hello</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:group -->

<!-- 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:columns {"className":"gutenberg-landing\u002d\u002ddevelopers-columns"} -->
<div class="wp-block-columns has-2-columns gutenberg-landing--developers-columns">
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left"><strong>Built with modern technology.</strong></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left">Gutenberg was developed on GitHub using the WordPress REST API, JavaScript, and React.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left","fontSize":"small"} -->
<p class="has-text-align-left has-small-font-size"><a href="https://wordpress.org/gutenberg/handbook/language/">Learn more</a></p>
<!-- /wp:paragraph -->
</div>
<!-- /wp:column -->
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left"><strong>Designed for compatibility.</strong></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left">We recommend migrating features to blocks, but support for existing WordPress functionality remains. There will be transition paths for shortcodes, meta-boxes, and Custom Post Types.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left","fontSize":"small"} -->
<p class="has-text-align-left has-small-font-size"><a href="https://wordpress.org/gutenberg/handbook/reference/faq/">Learn more</a></p>
<!-- /wp:paragraph -->
</div>
<!-- /wp:column -->
</div>
<!-- /wp:columns -->

<!-- wp:paragraph -->
<p>Hello</p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:group -->

<!-- wp:columns {"className":"gutenberg-landing\u002d\u002ddevelopers-columns"} -->
<div class="wp-block-columns has-2-columns gutenberg-landing--developers-columns">
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left"><strong>Built with modern technology.</strong></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left">Gutenberg was developed on GitHub using the WordPress REST API, JavaScript, and React.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left","fontSize":"small"} -->
<p class="has-text-align-left has-small-font-size"><a href="https://wordpress.org/gutenberg/handbook/language/">Learn more</a></p>
<!-- /wp:paragraph -->
</div>
<!-- /wp:column -->
<!-- wp:column -->
<div class="wp-block-column">
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left"><strong>Designed for compatibility.</strong></p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left"} -->
<p class="has-text-align-left">We recommend migrating features to blocks, but support for existing WordPress functionality remains. There will be transition paths for shortcodes, meta-boxes, and Custom Post Types.</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"align":"left","fontSize":"small"} -->
<p class="has-text-align-left has-small-font-size"><a href="https://wordpress.org/gutenberg/handbook/reference/faq/">Learn more</a></p>
<!-- /wp:paragraph -->
</div>
<!-- /wp:column -->
</div>
<!-- /wp:columns -->

selected & nested	selected & not nested	unselected

[iamthomasbishop]

@lukewalczak I think it’d probably make sense for the empty group appended to align to the same key lines as the filled one, but it’s not a blocker esp because it doesn’t conflict with a child outline (considering it doesn’t have siblings alongside it). So we can attack that in a separate issue if need be.

The last screenshots also look solid 👍 I can’t recall if there was an explicit decision on changing the selected/not-nested border, but I think we should eventually apply the same slight margin/inset that we use on the selected/nested one. Again this can be done separately, should not be considered a blocker on this.