[RNMobile] Inner Block Navigate down through hierarchy #17547

jbinda · 2019-09-24T09:34:09Z

Description

PR is connected with #1314 in gutenberg-mobile.

Please also refer to:

Related gutenberg-mobile PR
merge this PR first [RNMobile] FloatingToolbar and [RNMobile] Inner Block Navigate upward through hierarchy

It presents ability to select all parents in cascade way (according to the way how selecting in groups works on web version)

Please also refer to below description of the logic behind the styles which is applied to the blocks

Details

During selection the main rules are:

Content of each block should be align to same base
Selected block gets fully or semi solid blue border
Each first child of selected block gets grey dashed border around
Each block that is not selected gets dimmed if selection is nested (selected block is a child of other block)
Press on nested block always redirect to select first child of common ancestor with current selected block

1. Blocks on RootListLevel

when a block is selected and do not have inner blocks

- apply semi-border style with `.semiSolidBordered`
- apply regular margins with `.selectedRootLeaf` style

when a block is selected and have or could have inner blocks

- apply fully border style with `.fullSolidBordered`
- apply narrowed padding with `.selectedParent` style

when a block is not selected and do not have inner blocks

- do not have any border
- apply regular margins with `.full` style
- apply`.dimmed` style when any child of other InnerBlock is selected

when a block is not selected and have or could have inner blocks

- do not have any border
- apply no margins with `.neutral ` style
- depending on nesting level each block that is inside should be aligned to the same line with `.full` style

2. Blocks that renders InnerBlock inside (a component that can have inner block inside e.g. Group or MediaText)

InnerBlock is the only one child of component (e.g. Group)
```
- it should follow the rules as described in point 3
```

InnerBlock is not the only one child of component (e.g. MediaText)

- it should follow the rules as described in point 3
- exception is that the rest child do not gets dimmed

3. Block that is a descendant of InnerBlock component

when a parent is selected :

- apply dashed border with `.dashedBordered` style
- apply narrowed margin and no padding with `.childOfSelected` style if it `has` children
- apply narrowed margin/padding `.childOfSelectedLeaf` style if it `does not have` children

when ancestor is selected

- apply narrowed margins with ‘.descendantOfSelectedLeaf` style
- apply no margin with `.marginHorizontalNone` style when block has children

when a block is selected

- apply full border with `.blockHolderFullBordered` style
- apply narrowed margins and no padding with `.selectedParent` style if block `has` children
- apply narrowed margins and padding with  `.selectedLeaf` style if block `does not have` children

LEGEND:

_variables.scss
$block-edge-to-content: 16px;
$block-selected-margin: 3px;
$block-selected-border-width: 1px;
$block-selected-padding: 0;
$block-selected-child-margin: 5px;
$block-selected-child-border-width: 1px;
$block-selected-child-padding: 0;
$block-selected-to-content: $block-edge-to-content - $block-selected-margin - $block-selected-border-width;
$block-selected-child-to-content: $block-selected-to-content - $block-selected-child-margin - $block-selected-child-border-width;
$block-custom-appender-to-content: $block-edge-to-content - $block-selected-margin - $block-selected-child-margin + $block-selected-border-width;

block.native.scss
semi-border:

.semiSolidBordered {
	border-top-width: $block-selected-border-width;
	border-bottom-width: $block-selected-border-width;
	border-left-width: 0;
	border-right-width: 0;
	border-style: solid;
}

fully-border:

..fullSolidBordered {
 {
	border-width: $block-selected-border-width;
	border-radius: 4px;
	border-style: solid;
}

dashed-border:

...dashedBordered {
 {
	border-width: $block-selected-child-border-width;
	border-radius: 2px;
	border-style: dashed;
}

neutral:

.neutral {
	margin: 0;
	border: 0;
	padding: 0;
}

full:

.full {
	margin: $block-edge-to-content;
	border: 0;
	padding: 0;
}

selectedLeaf:

.selectedLeaf {
	margin: $block-selected-margin;
	padding: $block-selected-to-content;
}

selectedRootLeaf:

.selectedRootLeaf {
	margin: 0;
	padding: $block-edge-to-content;
}

selectedParent:

.selectedParent {
	margin: $block-selected-margin;
	padding: 0;
}

dimmed:

.childOfSelected {
	margin: $block-selected-child-margin;
	padding: 0;
}

dimmed:

.childOfSelectedLeaf {
	margin: $block-selected-child-margin;
	padding: $block-selected-child-to-content;
}

dimmed:

.descendantOfSelectedLeaf {
	margin: $block-selected-child-to-content;
}

dimmed:

.dimmed {
	opacity: 0.2;
}

How has this been tested?

You can use below initial-html to test nested grouping with applied border styling

`initial-html`

/**
 * @format
 * @flow
 */

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

<!-- wp:media-text -->
<div class="wp-block-media-text is-stacked-on-mobile"><figure class="wp-block-media-text__media"></figure><div class="wp-block-media-text__content"><!-- wp:paragraph {"placeholder":"Content…","fontSize":"large"} -->
<p class="has-large-font-size"></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text -->

<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:image {"id":1} -->
<figure class="wp-block-image"><img src="https://cldup.com/cXyG__fTLN.jpg" alt="" class="wp-image-1"/></figure>
<!-- /wp:image -->

<!-- wp:heading -->
<h2>Test Heading</h2>
<!-- /wp:heading -->

<!-- wp:video {"id":2} -->
<figure class="wp-block-video"><video controls src="https://i.cloudup.com/YtZFJbuQCE.mov"></video></figure>
<!-- /wp:video -->

<!-- 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:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:heading -->
<h2>Group Inner</h2>
<!-- /wp:heading -->

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

<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:heading -->
<h2>Group Inner 2</h2>
<!-- /wp:heading -->

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

<!-- wp:group -->
<div class="wp-block-group"><div class="wp-block-group__inner-container"><!-- wp:heading -->
<h2>Group Inner 3</h2>
<!-- /wp:heading -->

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

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

<!-- wp:media-text -->
<div class="wp-block-media-text is-stacked-on-mobile"><figure class="wp-block-media-text__media"></figure><div class="wp-block-media-text__content"><!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text --></div></div>
<!-- /wp:group -->

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

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

<!-- wp:media-text -->
<div class="wp-block-media-text is-stacked-on-mobile"><figure class="wp-block-media-text__media"></figure><div class="wp-block-media-text__content"><!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:media-text --></div></div>
<!-- /wp:group -->

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

Click through structure to see how dimmed and border style is applied for blocks

Screenshots

Initial GIF

Previous state:

**1:**

2:

Current state:

Types of changes

add getLowestCommonAncestorWithSelectedBlock selector to get blockId that is common ancestor of given block and currently selected block
apply opacity and dashed border according to given design depending of current selection and inner block structure
change onFocus handler to select block in parent hierarchy first

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 · 2019-11-28T13:55:00Z

Thanks a lot for the update @jbinda

At this point I'd like to also invite @Tug to give this a look as well as he introduced the InnerBlocks solution on mobile side and this is the next big iteration on top of it.

One more issue that currently can be noticed is when RichText is selected and user change selection by pressing on media placeholder. After close modal the selection will be back to RichText. It is something that is also on current develop branch. Please see GIF:

Correct. This is how it works on develop as well. It is about how modals interact with the block editor on iOS currently, i don't think we should try to solve this within this PR.

Tug

Approach looks good, I only had a few comments about code clarity. But generally, I would be fine going with this and iterate on the code and UX in a follow up PR.

I'll spend more time reviewing it see if there are other details I missed, would be nice to see the improvements I talked about addressed in the meantime :)

packages/block-editor/src/store/selectors.js

packages/block-editor/src/components/block-list/block.native.js

Tug · 2019-11-28T16:25:13Z

packages/block-editor/src/components/block-list/block.native.js

+		const isDescendantSelected = selectedBlockClientId && getBlockParents( selectedBlockClientId ).includes( clientId );
+		const isDescendantOfParentSelected = selectedBlockClientId && getBlockParents( selectedBlockClientId ).includes( parentId );
+		const isTouchable = isSelected || isDescendantSelected || isDescendantOfParentSelected || isParentSelected || parentId === '';
+		const isDimmed = ! isSelected && selectionIsNested && ! isAncestorSelected && ! isDescendantSelected && ( isDescendantOfParentSelected || rootBlockId === clientId );


Those conditions looks quite hard to understand! Could we try to make them more readable? I haven't spent that much time trying to understand it yet so I might have omitted some edge cases, but from the few cases I can think of, the following conditions would be more readable imo:

const isBlockHighlighted = selectionIsNested && ( isSelected || isParentSelected ); const isDimmed = ! isBlockHighlighted;

I'm not sure how isDescendantOfParentSelected could play a role here, is there a case where a component is highlighted when a "sibling" is selected?

As I understand, isDimmed should only be true for blocks that are outside the selection and its descendants, right? There was a suggestion to do a different think, but as far as I understand we didn't decide to go that way. It's not clear to me if we want to dim other blocks when a top level one is selected (even if it has children), or just when we start going 1+ level deep into nested blocks.

There are some redundant checks:

If isDescendantSelected ⇒ isDescendantOfParentSelected. So isDescendantSelected || isDescendantOfParentSelected == isDescendantSelected

I'm not sure how isDescendantOfParentSelected could play a role here, is there a case where a component is highlighted when a "sibling" is selected?

In my version it's the case when you select first child of root block. Without this check the rest of the components stays highlighted.

Next thing is that I need to dim only the bottom most descendant in each branch which should be dimmed to avoid multiplication of opacity applied to block in hierarchy. In other words if I apply opacity to block parent which fits criteria to be dimmed and them apply opacity for the block itself the block will have opacity 0.4 instead of desire 0.2. This opacity increased depending of nesting level. I hope that make sense

There is also edge case that do not dim when RootList block is selected as well.

I gave a shot for what you proposed and it does not works as expected but I will try to refactor original checks to increase clarity and write some comments in the code

@koke I have noticed your comment now so I want to refer to what you wrote as well

There are some redundant checks:
If isDescendantSelected ⇒ isDescendantOfParentSelected. So isDescendantSelected || isDescendantOfParentSelected == isDescendantSelected

That's true according to same branch.

The case for check 'isDescendantOfParentSelected' according to below image is when we have 1-4 selected (red) and isTouchable should be set to true for 1-2, 1-5. Also 1-4-5 ,1-4-3 should be touchable(green). The blue one circle represents block that should be dimmed.

I agree that we can simplify this:
const isTouchable = isSelected || isDescendantSelected || isDescendantOfParentSelected || isParentSelected || parentId === '';
to this:
const isTouchable = isSelected || isDescendantOfParentSelected || isParentSelected || parentId === '';

1-5 should have isDescendantOfParentSelected true and isDescendantSelected false because 1-4 is not descendant of 1-5 but is descendant of 1-5 parent

That's right, I knew one of them was redundant but I got my logic backwards 😅

I'm still not sure about isDimmed and try to check if we can simplify

packages/block-editor/src/components/block-list/block.native.js

packages/block-library/src/media-text/edit.native.js

jbinda · 2019-11-29T08:47:53Z

Hi @Tug

Thanks for review. I response on comments you gave and make updates

Tug

Thank you @jbinda comments really help in that case I think.

I find the dimmed/highlighted condition is still a bit complicated. As you discussed with Koke there are probably simplifications that could be made in that area.

Regarding the general UX I think this is an improvement from what we currently have so even if it could benefit some refinements, I'm all for merging it as it is now!

💯 🎉

jbinda · 2019-11-29T17:15:59Z

@Tug thanks for approval !

I need to check why CI fails on gutenberg-mobile and wait also for @koke approve.

After that I finally merge this feature ! :)

🎉 🎉

packages/block-editor/src/store/selectors.js

packages/block-editor/src/store/test/selectors.js

packages/block-library/src/media-text/edit.native.js

koke

I think this is looking pretty good and I'd be happy to merge after addressing the few minor things I commented. 👏 👏 So happy to see this one being ready, it's been quite the effort 😅

Co-Authored-By: Jorge Bernal <jbernal@gmail.com>

jbinda · 2019-12-03T08:52:09Z

@koke thank you for approval. I fixed thing you have pointed

jbinda · 2019-12-03T10:00:12Z

gutenberg repoCI pass, but still have some issue with passing gutenberg-mobile PR and will open thread on Slack to solve it

packages/block-editor/src/components/block-list/block.native.js

dratwas · 2019-12-05T13:54:19Z

packages/block-editor/src/components/block-list/block.native.js

+		const isSelectedBlockNested = !! getBlockRootClientId( selectedBlockClientId );
+
+		const isDescendantSelected = selectedBlockClientId && getBlockParents( selectedBlockClientId ).includes( clientId );
+		const isDescendantOfParentSelected = selectedBlockClientId && getBlockParents( selectedBlockClientId ).includes( parentId );


WDYT about create const with getBlockParents( selectedBlockClientId ) and use it instead of calling it 2 times?

After reading this comment, it did look a bit related to the firstToSelect search, so I think we could have the equivalent (please double check my logic here 😉):

isDescendantSelected: if the selection is one of the descendants, then this must be a parent of the selection, which also means it will be the lowest common ancestor. So isDescendantSelected = commonAncestor === clientId

isDescendantOfParentSelected: similar to the previous one, if the parent of this block is an ancestor of the selection, but the selection is not one of this block's descendants, then the lowest common ancestor if this block's parent: isDescendantOfParentSelected = commonAncestor === parentId

@koke I temporary go with @dratwas suggest.

However I agree with this one isDescendantSelected = commonAncestor === clientId

but not sure about this
isDescendantOfParentSelected = commonAncestor === parentId

Shouldn't we check the common ancestor with parent of selected block ?

Then it will look like this:
const commonAncestorParent = getLowestCommonAncestorWithSelectedBlock( parentId );
const isDescendantOfParentSelected = commonAncestorParent === parentId;

Right, I think that would change the behavior a bit. The commonAncestor == parentId would not include the case where isDescendantSelected == true. It would be more accurate to say isDescendantOfSiblingSelected, which might not be what you had in mind when doing the calculations. I think we can combine both:

const isDescendantSelected = commonAncestor === clientId; const isDescendantOfParentSelected = isDescendantSelected || commonAncestor === parentId;

But at this stage, this feels more like nitpicking, so I'm happy with either approach, we shouldn't block on this one to merge.

packages/block-editor/src/components/block-list/block.native.js

packages/block-editor/src/components/block-list/index.native.js

packages/block-editor/src/components/button-block-appender/index.native.js

packages/block-library/src/media-text/edit.native.js

jbinda requested review from ajitbohra, chrisvanpatten, ellatrix, gziolo, jorgefilipecosta, talldan and youknowriad as code owners September 24, 2019 09:34

This was referenced Sep 24, 2019

Feat: Navigation Down in InnerBlocks wordpress-mobile/gutenberg-mobile#1379

Merged

Inner Blocks - Navigating Down in the hierarchy wordpress-mobile/gutenberg-mobile#1314

Closed

gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Sep 26, 2019

pinarol assigned jbinda Sep 26, 2019

jbinda added the [Status] In Progress Tracking issues with work in progress label Oct 8, 2019

jbinda requested review from adamsilverstein, aduth, atimmer, daniloercoli, dmsnell, etoledom, karmatosed, mapk, mkaz, mmtr, nerrad, noisysocks, oandregal, ntwb, SergioEstevao, Soean, swissspidy and TimothyBJacobs as code owners October 8, 2019 11:13

pinarol requested a review from Tug November 28, 2019 13:55

Tug reviewed Nov 28, 2019

View reviewed changes

refactor after tug review

6414b89

Tug approved these changes Nov 29, 2019

View reviewed changes

koke reviewed Dec 2, 2019

View reviewed changes

koke approved these changes Dec 2, 2019

View reviewed changes

jbinda and others added 5 commits December 3, 2019 08:52

Update packages/block-editor/src/store/selectors.js

524e0ca

Co-Authored-By: Jorge Bernal <jbernal@gmail.com>

Update packages/block-editor/src/store/selectors.js

7cf3950

Co-Authored-By: Jorge Bernal <jbernal@gmail.com>

Update packages/block-library/src/media-text/edit.native.js

5a456c9

Co-Authored-By: Jorge Bernal <jbernal@gmail.com>

export styles for media-text to stylesheet

061257a

fix order in test case

7fb084a

jbinda force-pushed the rnmobile/navigate-down branch from 5a456c9 to 6414b89 Compare December 3, 2019 08:31

merge master

583650b

refactor stylesheet in mediatext

4574daf

group appender logic

22d5904

dratwas reviewed Dec 5, 2019

View reviewed changes

jbinda added 3 commits December 6, 2019 14:51

@dratwas CR refactor

75482c5

@dratwas CR refactor on descendant

0814bb8

Merge remote-tracking branch 'origin/master' into rnmobile/navigate-down

7cb1ddb

jbinda mentioned this pull request Dec 9, 2019

[RNMobile] Column block #19013

Merged

6 tasks

lukewalczak merged commit fdd6953 into WordPress:master Dec 11, 2019

lukewalczak deleted the rnmobile/navigate-down branch December 11, 2019 08:38

pinarol mentioned this pull request Dec 17, 2019

[RNMobile] Long-press on inserter to show options for "add above" and "add below" #18791

Merged

6 tasks

youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020

jbinda mentioned this pull request Jan 21, 2020

Adjust Navigation Down bordering logic to avoid vertical margins multiplication when nesting wordpress-mobile/gutenberg-mobile#1789

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Inner Block Navigate down through hierarchy #17547

[RNMobile] Inner Block Navigate down through hierarchy #17547

jbinda commented Sep 24, 2019 •

edited by pinarol

Loading

pinarol commented Nov 28, 2019

Tug left a comment

Tug Nov 28, 2019

koke Nov 29, 2019

jbinda Nov 29, 2019

jbinda Nov 29, 2019 •

edited

Loading

koke Nov 29, 2019

jbinda Nov 29, 2019

jbinda commented Nov 29, 2019

Tug left a comment

jbinda commented Nov 29, 2019 •

edited

Loading

koke left a comment

jbinda commented Dec 3, 2019

jbinda commented Dec 3, 2019

dratwas Dec 5, 2019

koke Dec 5, 2019

jbinda Dec 6, 2019

koke Dec 10, 2019

[RNMobile] Inner Block Navigate down through hierarchy #17547

[RNMobile] Inner Block Navigate down through hierarchy #17547

Conversation

jbinda commented Sep 24, 2019 • edited by pinarol Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

pinarol commented Nov 28, 2019

Tug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbinda Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbinda commented Nov 29, 2019

Tug left a comment

Choose a reason for hiding this comment

jbinda commented Nov 29, 2019 • edited Loading

koke left a comment

Choose a reason for hiding this comment

jbinda commented Dec 3, 2019

jbinda commented Dec 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbinda commented Sep 24, 2019 •

edited by pinarol

Loading

jbinda Nov 29, 2019 •

edited

Loading

jbinda commented Nov 29, 2019 •

edited

Loading