Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NavigationLink: fix getting Navigation parent block #20032

Merged

Conversation

@retrofox
Copy link
Contributor

retrofox commented Feb 4, 2020

Description

This PR fixes an issue which was found by @getdave reviewing this PR):


...However, I noticed a potential coding issue where we're expecting the rootBlock to always be the Nav Block:

const rootBlock = getBlockParents( clientId )[ 0 ];

...when in actual fact I think it could actually be any parent Block that uses InnerBlocks (eg: Columms) as it will return the topmost Block in the hierarchy. Therefore if the Block is nested then this logic is problematic.

I tested this theory by:

  • Adding a Column Block
  • Add Nav Block within the first Column
  • Setting Custom Colors
  • Looking for style attribute set on the DOM on the Nav Link.

It appears in the above scenario it doesn't work as the style wasn't applied to the Navigation Link Block, only to the parent Navigation Block.

Screen Shot 2020-02-04 at 13 41 11


So, in order to fix this issue what we do is adding a new getBlockParentsByBlockName() selector, which returns an array of parent client IDs, but also filtered by block name. So in this way, will ensure to get the proper attributes of the parent block:

const parentNavigationIds = getBlockParentsByBlockName(
	clientId,
	'core/navigation'
);

Then it's a matter of getting the first item of the array and pick up the block attributes.

How has this been tested?

  1. Adding a Column Block
  2. Add Nav Block within the first Column
  3. Setting Custom Colors
  4. Looking for style attribute set on the DOM on the Nav Link.

before
5) The colors attributes of the Navigation block are not propagated to the NavigationLink and the colors are not applied to the sub-menus:

Screen Shot 2020-02-04 at 10 52 25 AM

after
6) The colors are righty applied to the NavigationLink blocks

Screen Shot 2020-02-04 at 10 49 48 AM

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

This comment has been minimized.

Copy link
Contributor

marekhrabe commented Feb 4, 2020

I've made a testing document to test this also outside the Navigation. Made Columns > Column > Cover > Paragraph and tried:

> currentBlock = wp.data.select('core/block-editor').getSelectedBlock().clientId
"203aef60-1cbc-466e-85a1-f98935188e53" // the deepest block - Paragraph

> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/cover')
["b5740327-7582-49c5-920e-01199b1fbbd7"] // correctly returned parent Cover


> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/columns')
["8086e200-37d5-4890-ab8d-880d587f17a8"] // correctly returned parent Columns


> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/image')
[] // empty because there is no Image in parent list
Copy link
Contributor

marekhrabe left a comment

This is working well for me in separate testing and also in the implementation in Navigation. Colors are passed properly, as described

@retrofox retrofox mentioned this pull request Feb 4, 2020
4 of 4 tasks complete
@retrofox retrofox merged commit aae6d1b into master Feb 4, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Feb 4, 2020
@retrofox retrofox deleted the update/add-get-block-parents-by-block-name-selector branch Feb 4, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 4, 2020
ascending = false
) => {
const parents = getBlockParents( state, clientId, ascending );
return map(

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 5, 2020

Contributor

This selector will return new array on each call potentially causing performance issues, should we use createSelector?

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 5, 2020

Author Contributor

re-implemented here #20057

} = select( 'core/block-editor' );
const { clientId } = ownProps;
const rootBlock = getBlockParents( clientId )[ 0 ];
const rootBlock = head(
getBlockParentsByBlockName( clientId, 'core/navigation' )

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 5, 2020

Contributor

If I'm not mistaken, thecore/navigation block is always the direct parent to navigaton-link which means it's the last item in getBlockParents(clientId)

can't we solve this by just doing last(getBlockParents( clientId )) instead of introducing a new API (selector)?

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 5, 2020

Author Contributor

The issue happens when the Navigation gets nested items/submenus. Picking up just the first/last item is not enough.
image

chipsnyder added a commit that referenced this pull request Feb 7, 2020
* block-editor: add getBLockParentsByBlockName()

* docs: generate selectors doc

* navigation: get the proper parent block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Navigation block
  
✅ Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.