Update: Use new action REPLACE_INNER_BLOCKS for InnerBlocks replacement #14291
Conversation
jorgefilipecosta
requested review from
gziolo,
noisysocks,
talldan and
youknowriad
as code owners
jorgefilipecosta
force-pushed
the
update/use-new-action-for-inner-blocks-replacement
branch
from
4537c62
to
a6afb8e
Compare
jorgefilipecosta
requested review from
ajitbohra,
chrisvanpatten,
mkaz,
oandregal and
notnownikki
as code owners
There was a problem hiding this comment.
Thanks for updating this PR. I don't quite know how blocks reducer works at the moment so I would need to invest some time to understand it better. Seeing the complexity here, it feels like it could use some unit testing for the new action integrated into the reducer.
| } else { | ||
| insertBlocks( blocks, undefined, clientId, templateInsertUpdatesSelection ); | ||
| } | ||
| replaceInnerBlocks( clientId, blocks, block.innerBlocks.length === 0 && templateInsertUpdatesSelection ); |
There was a problem hiding this comment.
block.innerBlocks.length === 0 - can be computed inside replaceInnerBlocks. Is there any reason it has to be placed here?
There was a problem hiding this comment.
The selection is managed on a reducer that does not have access to the blocks themselves. In order to the check in a reducer, we would need a top-level high order reducer. We don't have any top-level higher-order reducer yet. I'm not sure we want to open that precedent.
But the main reason was to make easier to debug state changes. Currently, if the replaceInnerFlags action updateSelection flag is true the selection is updated if it is false the selection is not updated. If this logic was inside the reducer if the flag was false the selection was not updated, but if the flag was true the selection may or may not be updated.
|
Hi @gziolo that you for the review, I agree that unit tests need to be added, I did not add them yet because I want to validate this approach before investing the time in the test creation. |
gziolo
added
[Package] Block editor
jorgefilipecosta
force-pushed
the
update/use-new-action-for-inner-blocks-replacement
branch
from
a6afb8e
to
a2a982a
Compare
|
The unit tests were added. |
jorgefilipecosta
added
the
[Status] In Progress
jorgefilipecosta
force-pushed
the
update/use-new-action-for-inner-blocks-replacement
branch
from
a2a982a
to
c765b6e
Compare
jorgefilipecosta
removed
the
[Status] In Progress
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
jorgefilipecosta
deleted the
update/use-new-action-for-inner-blocks-replacement
branch
|
Thank you for the review and suggestions @gziolo! |
Description
Adds a new action REPLACE_INNER_BLOCKS that is used during the inner block creation/update to set its template.
We are updating the insert/replace and move actions to restrict if the new blocks can be added in the specific locations. During the set of an inner blocks template even if restrictions exist e.g: locking, the blocks should be added, this would require flags in the existing actions to not apply restriction in that case.
@gziolo suggested that we use a new specific action, and this PR implements it.
Pending: test cases, after we check if this approach (use a High Order Reducer) is valid I will add them to this PR.
How has this been tested?
I used the columns block, changed the number of columns, and verified everything still works as before.
I used the Media & Text block and verified the heading block still does not get selected by default.
I tried a sample block with inner blocks https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js and checked the block still work as before.