Editor: Reimplement select previous/next as independent controls #13924
Conversation
aduth
requested review from
ajitbohra,
chrisvanpatten,
gziolo,
mkaz,
noisysocks,
oandregal,
notnownikki and
talldan
as code owners
| selectBlock( getPreviousBlockClientId( clientId ), -1 ); | ||
| } ); | ||
|
|
||
| export const SELECT_NEXT_BLOCK = createRegistryControl( ( registry ) => ( action ) => { |
There was a problem hiding this comment.
Can you explain why you chose to write these two actions as controls?
For me, I see controls as generic behavior (selecting, dispatching, rest api calls... but not something that looks more like an action)?
There was a problem hiding this comment.
Can you explain why you chose to write these two actions as controls?
For me, I see controls as generic behavior (selecting, dispatching, rest api calls... but not something that looks more like an action)?
From pull request originating comment:
I feel slightly uneasy about this implementation only because it seems to leverage controls for the mere convenient access to selectors to determine the next/previous block client IDs. I think an alternative approach could be one where the actions are handled entirely within the reducer. However, this would require that
blockSelectionhave some awareness of order, which is not presently true.
There was a problem hiding this comment.
As implemented, it's very much attaching behavior to an action where a need exists to consume data from the store. I find it similar (with all the bad feelings attached) to the Redux Thunk middleware exposing similarly the store as a second argument.
A problem we'll face is that this is arguably quote tempting to leverage.
I think there may be some possibility to my alternative proposal where a higher-order reducer wraps the base editor reducer, in order to share information from editor.blocks.order to blockSelection. As described by #12327, ideally these would be closer together anyways.
There was a problem hiding this comment.
Here's what I had in mind 32ea24f
There was a problem hiding this comment.
Here's what I had in mind 32ea24f
Oh, I hadn't even considered that. To be honest, it doesn't seem fundamentally different from what I've proposed as in they both leverage controls to access selectors, but as you've proposed certainly does make the implementation vastly simpler.
There was a problem hiding this comment.
The difference for me is that the "controls" should be generic things that can be shared in a reusable module across stores. (like redux middlewares or things like that)
aduth
force-pushed
the
update/block-select-previous-control
branch
from
22c865d
to
424604e
Compare
|
@youknowriad I cherry-picked your commit 0d460f0 with some additional revisions in 282f34e and 0b539b5 . I'm pretty happy with the current state of the pull request. |
There was a problem hiding this comment.
The current implementation worked well on my tests.
I like this approach and I think it can be applied in other similar situations, namely the logic to restrict block insertion and moving according to locking, allowed blocks, child blocks etc...
I'm going to close #7301 that solved the problem by using middleware and I'm going to propose something similar to this approach.
…cks, locking, and child blocks (#14003) Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files Uses the select control implemented in #13924. This PR changes the action creators to be a generator and have all the required validations before yielding the action. Fixes: #14568; Fixes: #10186; Fixes: #13099; Missing the update to the unit tests. ## How has this been tested? Test block: [gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js](https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js) I pasted the test block referred in the browser console. I added the Product block. I verified I cannot move using drag & drop the Buy button out of the Product block. (child block restriction. I verified I cannot move using drag & drop a paragraph created outside of the Product block inside the product block. (allowed blocks restriction). I verified that if I press enter in the buy button or image inside the product block, a paragraph is not created (allowed blocks restriction). I added two galleries outside the product block, multi-selected them, and copied them ( ctrl + c ), I pasted them inside the list block inside the Product block and I verified they were not added (allowed blocks restriction). I verified that when we have a template lock and we press enter on the title we now do not insert unallowed blocks. Template lock code: ``` add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) { if ( $post->post_type === 'post' ) { return $allowed_block_types; } return [ 'core/image', 'core/button' ]; }, 10, 2 ); ``` I verified that if we disable the paragraph, e.g., with a filter or inside the test block, pressing enter on the title or in placeholders does not add paragraph blocks. Pressing the sibling inserter also does not insert the paragraph (PR #7226). Code to disable paragraph: ``` function myplugin_register_book_lock_post_type() { $args = array( 'public' => true, 'template_lock' => 'all', 'label' => 'Books Lock', 'show_in_rest' => true, 'template' => array( array( 'core/image', array( ) ), array( 'core/heading', array( 'placeholder' => 'Add Author...', ) ), array( 'core/paragraph', array( 'placeholder' => 'Add Description...', ) ), ), ); register_post_type( 'book_lock', $args ); } add_action( 'init', 'myplugin_register_book_lock_post_type' ); ``` I added two image blocks, multi-selected them, copied them and tried to paste them on the list inside the test block. I checked the image blocks were not added inside the test block. All these actions are possible in master. Todo: If we accept this approach, end 2 end tests will be created before the merge that replicate this manual tests.
…cks, locking, and child blocks (#14003) Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files Uses the select control implemented in #13924. This PR changes the action creators to be a generator and have all the required validations before yielding the action. Fixes: #14568; Fixes: #10186; Fixes: #13099; Missing the update to the unit tests. ## How has this been tested? Test block: [gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js](https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js) I pasted the test block referred in the browser console. I added the Product block. I verified I cannot move using drag & drop the Buy button out of the Product block. (child block restriction. I verified I cannot move using drag & drop a paragraph created outside of the Product block inside the product block. (allowed blocks restriction). I verified that if I press enter in the buy button or image inside the product block, a paragraph is not created (allowed blocks restriction). I added two galleries outside the product block, multi-selected them, and copied them ( ctrl + c ), I pasted them inside the list block inside the Product block and I verified they were not added (allowed blocks restriction). I verified that when we have a template lock and we press enter on the title we now do not insert unallowed blocks. Template lock code: ``` add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) { if ( $post->post_type === 'post' ) { return $allowed_block_types; } return [ 'core/image', 'core/button' ]; }, 10, 2 ); ``` I verified that if we disable the paragraph, e.g., with a filter or inside the test block, pressing enter on the title or in placeholders does not add paragraph blocks. Pressing the sibling inserter also does not insert the paragraph (PR #7226). Code to disable paragraph: ``` function myplugin_register_book_lock_post_type() { $args = array( 'public' => true, 'template_lock' => 'all', 'label' => 'Books Lock', 'show_in_rest' => true, 'template' => array( array( 'core/image', array( ) ), array( 'core/heading', array( 'placeholder' => 'Add Author...', ) ), array( 'core/paragraph', array( 'placeholder' => 'Add Description...', ) ), ), ); register_post_type( 'book_lock', $args ); } add_action( 'init', 'myplugin_register_book_lock_post_type' ); ``` I added two image blocks, multi-selected them, copied them and tried to paste them on the list inside the test block. I checked the image blocks were not added inside the test block. All these actions are possible in master. Todo: If we accept this approach, end 2 end tests will be created before the merge that replicate this manual tests.
…cks, locking, and child blocks (#14003) Supersedes: https://github.com/WordPress/gutenberg/pull/7301/files Uses the select control implemented in #13924. This PR changes the action creators to be a generator and have all the required validations before yielding the action. Fixes: #14568; Fixes: #10186; Fixes: #13099; Missing the update to the unit tests. ## How has this been tested? Test block: [gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js](https://gist.github.com/jorgefilipecosta/b7194daac3ce26827522694fb4252c1c#file-testallowedblocksmiddleware-js) I pasted the test block referred in the browser console. I added the Product block. I verified I cannot move using drag & drop the Buy button out of the Product block. (child block restriction. I verified I cannot move using drag & drop a paragraph created outside of the Product block inside the product block. (allowed blocks restriction). I verified that if I press enter in the buy button or image inside the product block, a paragraph is not created (allowed blocks restriction). I added two galleries outside the product block, multi-selected them, and copied them ( ctrl + c ), I pasted them inside the list block inside the Product block and I verified they were not added (allowed blocks restriction). I verified that when we have a template lock and we press enter on the title we now do not insert unallowed blocks. Template lock code: ``` add_filter( 'allowed_block_types', function( $allowed_block_types, $post ) { if ( $post->post_type === 'post' ) { return $allowed_block_types; } return [ 'core/image', 'core/button' ]; }, 10, 2 ); ``` I verified that if we disable the paragraph, e.g., with a filter or inside the test block, pressing enter on the title or in placeholders does not add paragraph blocks. Pressing the sibling inserter also does not insert the paragraph (PR #7226). Code to disable paragraph: ``` function myplugin_register_book_lock_post_type() { $args = array( 'public' => true, 'template_lock' => 'all', 'label' => 'Books Lock', 'show_in_rest' => true, 'template' => array( array( 'core/image', array( ) ), array( 'core/heading', array( 'placeholder' => 'Add Author...', ) ), array( 'core/paragraph', array( 'placeholder' => 'Add Description...', ) ), ), ); register_post_type( 'book_lock', $args ); } add_action( 'init', 'myplugin_register_book_lock_post_type' ); ``` I added two image blocks, multi-selected them, copied them and tried to paste them on the list inside the test block. I checked the image blocks were not added inside the test block. All these actions are possible in master. Todo: If we accept this approach, end 2 end tests will be created before the merge that replicate this manual tests.
Extracted from #13088
This pull request seeks to refactor the implementation where removing a block should select the block preceding the removed. Previously, this had been implemented as a side-effect, determining the block to select by recreating the state at the prior point in history when the block had existed. This will not be possible in #13088, since the block editor module will not retain its own history.
The changes here reimplement the behavior as controls assigned to a new pair of actions
selectNextBlockandselectPreviousBlock. Technically only the latter is ever used, but the inverse is included for completeness.Implementation Notes:
I feel slightly uneasy about this implementation only because it seems to leverage controls for the mere convenient access to selectors to determine the next/previous block client IDs. I think an alternative approach could be one where the actions are handled entirely within the reducer. However, this would require that
blockSelectionhave some awareness of order, which is not presently true.I had thought perhaps there may be a way to
yieldorreturnan action object from the control implementation, as theselectBlockaction is local to the controls file and it's not strictly necessary to operate through the registry object. In my testing, however, this did not work.Testing instructions:
Verify there are no regressions in the behavior to select previous on remove block. Notably, you should remove a block via the block toolbar extended options menu, or using the keyboard shortcut.
This is also encompassed in the block deletion end-to-end tests, as additional confirmation: