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

BlockEditor: refactoring getBlockParentsByBlockName() selector #20057

Conversation

@retrofox
Copy link
Contributor

retrofox commented Feb 5, 2020

Description

It re implements the getBlockParentsByBlockName() selector using the createSelector() function to memoize what t returns in order to avoid performance issues.

How has this been tested?

Manually

Create the following block structure

  1. Create a column block (two columns)
  2. Add a Navigation block inside
  3. Add submenus
  4. Set customs colors for text and background

Visually, something like this:

image

or paste the following code in the Code Editor mode

<!-- wp:columns -->
<div class="wp-block-columns"><!-- wp:column -->
<div class="wp-block-column"><!-- wp:navigation {"rgbTextColor":"#6ab2d1","rgbBackgroundColor":"#051922","itemsJustification":"left"} -->
<!-- wp:navigation-link {"label":"\u003cstrong\u003eAbout\u003c/strong\u003e","title":"Sample Page","type":"page","id":2,"url":"http://localhost:8889/?page_id=2"} -->
<!-- wp:navigation-link {"label":"Contact"} -->
<!-- wp:navigation-link {"label":"Work with Us!"} -->
<!-- wp:navigation-link {"label":"Contact!"} /-->

<!-- wp:navigation-link {"label":"Twenty-Twenty"} /-->

<!-- wp:navigation-link {"label":"Maywood"} /-->

<!-- wp:navigation-link /-->
<!-- /wp:navigation-link -->
<!-- /wp:navigation-link -->

<!-- wp:navigation-link {"label":"Call Us!"} /-->

<!-- wp:navigation-link {"label":"Me"} /-->

<!-- wp:navigation-link {"label":"You!"} /-->
<!-- /wp:navigation-link -->

<!-- wp:navigation-link {"label":"Contact"} -->
<!-- wp:navigation-link {"label":"Regie Miller"} /-->

<!-- wp:navigation-link {"label":"John Stockton"} /-->

<!-- wp:navigation-link {"label":"Penny Hardaway "} /-->

<!-- wp:navigation-link /-->
<!-- /wp:navigation-link -->
<!-- /wp:navigation --></div>
<!-- /wp:column -->

<!-- wp:column -->
<div class="wp-block-column"></div>
<!-- /wp:column --></div>
<!-- /wp:columns -->

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

When you inspect the nested Navigation Items, the colors should be propagated to all of them.

Also, you can check the behavior following the steps of this comment.

Unit tests

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.
@retrofox retrofox requested a review from marekhrabe Feb 5, 2020
@retrofox retrofox changed the title block-editor: refat getBlockParentsByBlockName BlockEditor: refactoring getBlockParentsByBlockName() selector Feb 5, 2020
@retrofox retrofox mentioned this pull request Feb 5, 2020
5 of 5 tasks complete
@retrofox retrofox added this to Needs Review in WordPress 5.4 Must Have Feb 5, 2020
@retrofox retrofox requested a review from jorgefilipecosta Feb 5, 2020
Copy link
Contributor

marekhrabe left a comment

I've tested this for regressions the same way I tested the previous implementation: I 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

Still getting expected results for both manual testing and the integration in Navigation

( { id } ) => id
);
},
( state ) => [ state.blocks.parent ]

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 6, 2020

Contributor

If I'm not mistaken we only care about state.blocks.parent[clientId]

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 6, 2020

Author Contributor

Maybe I misunderstood the dependant references here. I set state.blocks.parent because the selector depends on its parent, so if they change, it needs to clear the cache and regenerates.
However, it's already handled by getBlockParents() selector. Is this correct?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 6, 2020

Contributor

Unless I'm wrong the returned value on change if the parents of a specific block (clientId) change, right?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 6, 2020

Contributor

Actually, I'm wrong state.blocks.parent[ clientId ] doesn't exist, neighter state.blocks.parent. It should be state.blocks.parents as we can target all block parents at once.

This comment has been minimized.

Copy link
@retrofox

retrofox Feb 6, 2020

Author Contributor

Beyond that .parent doesn't exist 🤦‍♂, is not already handled by the getBlockParents() selector?

oh, got it... we need to be totally specific about the dependant references. Since although it's true that the getBlockParents() selector cache depends on state.blocks.parents, if this selector depends just on the clientId, it's possible that it won't necessarily regenerate its cache if .parents tree changes.

Copy link
Contributor

getdave left a comment

Just noting that we're missing unit tests for this new selector.

@retrofox retrofox force-pushed the update/block-editor-re-implement-selector-with-create-selector branch from f832fcd to 2a0cb59 Feb 6, 2020
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 6, 2020

Just noting that we're missing unit tests for this new selector.

It's a very valid point. Going to try to add it asap.

retrofox added 3 commits Feb 5, 2020
It re implements the getBlockParentsByBlockName() selector using the createSelector() function to memoize what the selector returns in order to avoid performance issues.

block-editor: update dependant references in getBlockParentsByBlockName()

block-editor: fix
@retrofox retrofox force-pushed the update/block-editor-re-implement-selector-with-create-selector branch from 2a0cb59 to f1aac92 Feb 6, 2020
@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 6, 2020

Just noting that we're missing unit tests for this new selector.

Added unit tests for getBlockParents() and getBlockParentsByBlockName()

@retrofox

This comment has been minimized.

Copy link
Contributor Author

retrofox commented Feb 7, 2020

Thanks folks for the review. 🙇

@retrofox retrofox merged commit c6b019b into master Feb 7, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Feb 7, 2020
@retrofox retrofox deleted the update/block-editor-re-implement-selector-with-create-selector branch Feb 7, 2020
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

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