InnerBlocks: introduce prop to specify render callback for each block. #24232
Conversation
ZebulanStanphill
added
[Feature] Nested / Inner Blocks
|
Size Change: +174 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ZebulanStanphill
changed the title
|
Thanks for exploring this! Could you make something (e.g. the Navigation block) use this new API so that we can test how this approach affects things like drag and drop? I suspect there may be implications there. I'm hesitant about how many if specified, then render something props there are on // This is naive as it doesn't handle `key` and `ref` properly.
<InnerBlocks>
{ ( blocks ) => (
<ul>
{ blocks.map( ( block ) => ( <li>{ block }</li> ) ) }
<li><InnerBlocks.Appender /></li>
</ul>
) }
</InnerBlocks>(But let's first test if the approach works before worrying about a nice API.) |
|
@noisysocks For testing purposes, I have now modified the Group block to wrap each of its children in a Your HOC idea looks pretty interesting. I'm not sure how to actually implement that concept, but it looks promising, so if you want to spin up a PR using that approach, please do so. I think something like that may be the right way to go. |
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
6b1d45a to
490dbcf
Compare
|
I tested it, found some bugs, and fixed most of them. Here's what works:
Here's what still doesn't:
Additionally, the only failing tests right now are caused by the temporary testing change made to the Group block's markup, so that's a good sign. Notably, the up/down movers were already working prior to 490dbcf, but the animations were not. In fixing this, I ended up moving some of my wrapper logic from It should be noted that there is at least one major difference between the inner block item wrapper and the alignment wrapper: the former should not be considered part of the block, while the latter is (but should it)? The alignment wrapper (for most blocks anyway) only exists in the editor, while the alignment classes are just applied to the block's normal root element in the save implementation. Ideally, the alignment should happen the same way in both implementations. Notably, the Image block uses a custom implementation which uses a wrapper for alignment in both the editor and the front-end. To clarify, in my save implementation, the The editor situation is interesting since there are no comment delimiters, and as such, the lines between where a block starts and ends are a bit less obvious. Any thoughts/opinions on this? As for fixing the drag-and-drop issue, I'm not sure how to fix that yet. Any suggestions would be very welcome. |
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
2 times, most recently
from
f11b79a to
57c0dfa
Compare
|
Alright, drag-and-drop is fixed. I've also attempted to make this work with React Native. I have no clue if the native implementation works or how to test it (I can't even get the normal wp-env set up on my computer), so if anyone could test that for me, that would be great. |
|
I tested the web version locally and it works pretty well! Nice job @ZebulanStanphill. I think I'm happy with the approach. I like that it's a simple change, and the fact that we've encountered this problem in two seperate places reassures me that it's a problem worth solving at a framework level. I'd like it if the API were more flexible, as above, but nothing stopping us from iterating on that later seeing as this is all It's a bit of a paradigm shift, so I'd like to hear opinions from more @WordPress/gutenberg-core developers, particularly @ellatrix and @youknowriad, before proceeding. I note that it's linked to in the agenda of today's editor chat. |
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
57c0dfa to
514b785
Compare
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
514b785 to
b1c0e08
Compare
|
I have now switched to a callback-based API, as suggested by @ellatrix. You can now do stuff like this: <InnerBlocks
__experimentalItemCallback={ ( item ) => (
<>
<section>{ item }</section>
<p>You can do cool/weird stuff like this now.</p>
</>
) }
/>One thing that I've discovered while working on this PR is that the position of the drag-and-drop indicator line isn't ideal. It appears before the block it will be dropping before, but it doesn't account for any extra markup added by this callback API, so if you're applying wrappers with a lot of top padding or appending stuff before each block, the drop-indicator won't appear in the right spot. This is not a new problem; it exists in |
| // Get the root elements of blocks inside the element, ignoring | ||
| // InnerBlocks item wrappers and the children of the blocks. | ||
| const blockElements = difference( | ||
| Array.from( element.current.querySelectorAll( '.wp-block' ) ), | ||
| Array.from( | ||
| element.current.querySelectorAll( | ||
| ':scope .wp-block .wp-block' | ||
| ) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Noting that this may not be the fastest way to do this. Here's an alternative implementation that I've considered, but I have no idea if it would be faster or slower:
function getBlockElements( node ) {
const list = [];
for ( const childNode of node ) {
if ( childNode.classList.contains( 'wp-block' ) ) {
list.push( childNode );
} else if ( childNode.children.length > 0 ) {
const grandchildren = getBlockElements( childNode );
if ( grandchildren.length > 0 ) {
list.push( ...grandchildren );
}
}
}
return list;
}
const blockElements = getBlockElements( element.current );
There was a problem hiding this comment.
Probably they're quite similar. When does this code run? Unless it's a very critical path (e.g. every keystroke, every block render) it's probably not worth worrying about too much.
You could always measure it using console.time().
console.time( 'getBlockElements' );
// Code goes here
console.endTime( 'getBlockElements' );
| @@ -93,6 +93,7 @@ function BlockListBlock( { | |||
| toggleSelection, | |||
| index, | |||
| enableAnimation, | |||
| __experimentalCallback: wrapperCallback, | |||
There was a problem hiding this comment.
Without the __experimental prefix, this prop would just be named callback which is pretty ambiguous. Let's name it something like __experimentalBlockEditCallback.
There was a problem hiding this comment.
I decided to go with __experimentalRenderCallback. How's that?
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
b1c0e08 to
15e35db
Compare
|
I realized that non-light blocks had the render callback applied inside their |
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
15e35db to
312f800
Compare
ZebulanStanphill
changed the title
ZebulanStanphill
requested review from
ajitbohra,
Soean and
youknowriad
as code owners
Fixes block moving animations for inner blocks.
This makes it possible to switch to a callback-based API, e.g. "itemWrapper={ ( item ) => <div>{ item }</div> }".
Edit implementation:
<InnerBlocks
__experimentalItemCallback={
( item ) => <div>{ item }</div>
}
/>
Save implementation:
<InnerBlocks.Content
__experimentalItemCallback={
( item ) => <div>{ item }</div>
}
/>
Fixes non-light blocks having the render callback applied inside the Block component rather than outside. Also moves the "Edit as HTML" form of blocks inside the render callback.
getNearestBlockIndex now assumes all elements passed to it are the root elements of blocks; we filter out the other elements before passing the array to the function.
ZebulanStanphill
force-pushed
the
add/inner-blocks-child-wrapper-prop
branch
from
86fd186 to
7342bc1
Compare
|
As a follow-up: should we replace |
|
Uh-oh. While testing this feature out in #24039, I think I've discovered a major flaw in the current implementation... namely that I only made changes to serialization code, and I never changed any of the parsing code. This means that inserting a block using this API will work fine in the editor, and even saving it and viewing it on the front-end will work. But if you leave the editor and then return, it won't be able to parse the blocks using InnerBlock render callbacks, and block validation will fail.. I'm not really sure how to fix this, but it's a serious bug that renders the entire feature unusable. Any technical help would be greatly appreciated. |
|
Howdy @ZebulanStanphill. Is this feature used anywhere currently? In the navigation editor the problem ended up being solved differently. If there is no use case perhaps the best path for this API is to be removed via a revert? |
|
@draganescu The feature is currently being used in #24039, as I just mentioned. |
|
Oops! 😊 |
|
@ZebulanStanphill FYI - I am not too heavily invested in this approach in #24039, so if we need to back it out for now and explore alternatives that is fine, or also happy to wait until we get the parsing side sorted. |
|
@glendaviesnz I still think the render callback approach is the best one, because the only other solutions I am aware of are:
That said, it's probably best not to have a completely broken API in the code, even if it is experimental. And I am not currently aware of how to complete the implementation. So I've created #25196 to remove this API for now. |
|
Is there any plan to revisit this? It would be really useful for a mixed-media gallery I'm trying to build. |
Description
This PR adds an
__experimentalItemWrapperprop toInnerBlocksandInnerBlocks.Content, allowing blocks to specify a component to wrap each block. The feature is designed to help solve issues discussed in #23915 and #24039.Update 2020-08-05: this PR has been revised to use a more flexible callback-based API. This allows you to not only wrap all child blocks with multiple levels of parent elements, but also append stuff before and after each of them.
To pull this off, it also ends up modifying
BlockList,BlockListBlock,BlockContentProvider, andserializeBlock. The changes should have no effect on anything that doesn't use the new feature.Reasoning behind my decision to take this approach.
How has this been tested?
It has not yet been tested. Currently planning on testing this via #24039.
Todo
Checklist: