Fix widget insertion from sidebar block library by using a declared insertionPoint prop #25763
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nsertionPoint prop
talldan
commented
Oct 1, 2020
Comment on lines
27
to
42
| const blockSelectionEnd = getBlockSelectionEnd(); | ||
| const blockRootClientId = getBlockRootClientId( blockSelectionEnd ); | ||
| // getBlockRootClientId returns an empty string for top-level blocks, in which case just return the block id. | ||
| return blockRootClientId === '' ? blockSelectionEnd : blockRootClientId; | ||
| const blockSelectionEndClientId = getBlockSelectionEnd(); | ||
|
|
||
| if ( | ||
| getBlockName( blockSelectionEndClientId ) === 'core/widget-area' | ||
| ) { | ||
| return blockSelectionEndClientId; | ||
| } | ||
|
|
||
| const blockParents = getBlockParents( blockSelectionEndClientId ); | ||
| const rootWidgetAreaClientId = blockParents.find( | ||
| ( clientId ) => getBlockName( clientId ) === 'core/widget-area' | ||
| ); | ||
|
|
||
| if ( rootWidgetAreaClientId ) { | ||
| return rootWidgetAreaClientId; | ||
| } |
There was a problem hiding this comment.
I made some changes here, as this is used with the setIsWidgetAreaOpen function, but previously could return a rootClientId for a block that's not a widget area (e.g. a group block).
6 tasks
youknowriad
reviewed
Oct 1, 2020
| @@ -46,6 +47,7 @@ function InserterMenu( { | |||
| clientId, | |||
| isAppender, | |||
| selectBlockOnInsert: __experimentalSelectBlockOnInsert, | |||
| insertionIndex: __experimentalInsertionIndex, | |||
There was a problem hiding this comment.
I'm concerned about the proliferation of inserter props that all have the same intent, chose where to insert block: isAppender, __experimentalSelectBlockOnInsert, __experimentalInsertionIndex, rootClientId and clientId are all related to that. I wonder if it's time to take a step back here and figure out something consistent.
There was a problem hiding this comment.
Yeah, I get that it's adding more along the same lines, especially clientId and __experimentalInsertionIndex, which are very similarly used.
We could continue making the insertion index more deterministic (more logic inside of useInsertionPoint). An option there could be to consider that the root block list in widgets it essentially locked, so maybe useInsertionPoint could detect that and instead insert inside the selected block when possible. I'm not sure how that would feel in terms of user experience.
Or alternatively continue on the path of making it configurable, but in a nicer way, make the entire useInsertionPoint hook replaceable?
There was a problem hiding this comment.
Or alternatively continue on the path of making it configurable, but in a nicer way, make the entire useInsertionPoint hook replaceable?
Yes, something like that might make sense.
I also wonder whether we should remove the responsibility of choosing the position from the inserter component and just have a prop like { rootClientId + index } with index being optional and equal to the last position?
There was a problem hiding this comment.
That sounds good. I'll do some exploration in a separate PR (this one just merges into #25727).
|
Size Change: +189 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
kevin940726
approved these changes
Oct 5, 2020
Comment on lines
24
to
42
| const { getBlockSelectionEnd, getBlockParents, getBlockName } = select( | ||
| 'core/block-editor' | ||
| ); | ||
| const blockSelectionEnd = getBlockSelectionEnd(); | ||
| const blockRootClientId = getBlockRootClientId( blockSelectionEnd ); | ||
| // getBlockRootClientId returns an empty string for top-level blocks, in which case just return the block id. | ||
| return blockRootClientId === '' ? blockSelectionEnd : blockRootClientId; | ||
| const blockSelectionEndClientId = getBlockSelectionEnd(); | ||
|
|
||
| if ( | ||
| getBlockName( blockSelectionEndClientId ) === 'core/widget-area' | ||
| ) { | ||
| return blockSelectionEndClientId; | ||
| } | ||
|
|
||
| const blockParents = getBlockParents( blockSelectionEndClientId ); | ||
| const rootWidgetAreaClientId = blockParents.find( | ||
| ( clientId ) => getBlockName( clientId ) === 'core/widget-area' | ||
| ); | ||
|
|
||
| if ( rootWidgetAreaClientId ) { | ||
| return rootWidgetAreaClientId; | ||
| } |
There was a problem hiding this comment.
I think we can use some comments here :)
|
Thanks for the review @kevin940726. I added the comments in a6bf31a, and also decided to reorder some of the code in 5f8c61b. Merging into your PR now. |
adamziel
pushed a commit
that referenced
this pull request
Oct 6, 2020
* Move hooks inside the newly created Interface component * Fix insertion at the top of the widget area * Fix widget insertion from sidebar block library by using a declared insertionPoint prop (#25763) * Fix widget insertion from sidebar block library by using a declared insertionPoint prop * Add comments * Optimize by avoiding `getEntityRecord` call unless needed Co-authored-by: Daniel Richards <[email protected]>
adamziel
pushed a commit
that referenced
this pull request
Oct 7, 2020
* Move hooks inside the newly created Interface component * Fix insertion at the top of the widget area * Fix widget insertion from sidebar block library by using a declared insertionPoint prop (#25763) * Fix widget insertion from sidebar block library by using a declared insertionPoint prop * Add comments * Optimize by avoiding `getEntityRecord` call unless needed Co-authored-by: Daniel Richards <[email protected]>
draganescu
pushed a commit
that referenced
this pull request
Oct 7, 2020
* Include edit-widgets php files in build (#25792) * Fix PHP warining in widget utils controller (#25797) * Fix PHP warning in WP_REST_Widget_Utils_Controller The `WP_REST_Widget_Utils_Controller::is_valid_widget` method needs to be public in order to be accessible as a callback (since it's being called from outside the class, via `call_user_func`). This commit fixes the warning by changing the method's visibility from `private` to `public`. * Ammend the inline documentation. Add `* @access public` * [Widgets Editor] Fix insertion point in widget areas (#25727) * Move hooks inside the newly created Interface component * Fix insertion at the top of the widget area * Fix widget insertion from sidebar block library by using a declared insertionPoint prop (#25763) * Fix widget insertion from sidebar block library by using a declared insertionPoint prop * Add comments * Optimize by avoiding `getEntityRecord` call unless needed Co-authored-by: Daniel Richards <[email protected]> * Initialize the state before rendering widgets editor (#25736) * Initialize the state before rendering widgets editor * Replace empty div with null * Document persistStubPost * Document persistStubPost further * Bump version to 9.1.1 * Update changelog * Fix spaces in changelog.txt * Adjust spaces in changelog.txt * Fix link formatting in the changelog Co-authored-by: Jon Surrell <[email protected]> Co-authored-by: David Biňovec <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Daniel Richards <[email protected]>
talldan
added
[Feature] Widgets Screen
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
An alternative way to handle #25727 (this PR merges into that one).
Primer
Insertion on the widgets page is somewhat unique in comparison to the post editor. There are multiple
widget-areablocks that act as roots for block insertion.The trouble is that when inserting from the library, the default behavior is to insert after the selected block, but for these widget areas, we always want to insert inside of them.
This PR allows for the definition of an
insertionIndexfor the blockLibrarycomponent. When defined it overrides the behavior the library has where it defaults to inserting after the selection.How has this been tested?
With a widget area selected
With a block inside a widget area selected
Screenshots
Types of changes
Bug fix
Checklist: