Lighter InnerBlocks: allow blocks to render own wrappers #19910
Conversation
Incredible. Let's let that sink in for a moment. I didn't think this was possible, but it appears you may have done it. Obviously this is a big change, so it needs many eyes and reviews, and there are likely a ton of edgecases and things that will need touching up a a result of such a big refactor. But before diving into that, it seems worth listing why this matters, what the benefits are:
In other words, there are benefits to code quality, to developers, to themers, and to end users who will be more likely to use themes that look like the front-end, increasing usability. That's all to say that this PR is worth reviewing and helping to completion! |
ellatrix
force-pushed
the
try/block-wrapper
branch
from
732e3f1
to
05be98d
Compare
ellatrix
force-pushed
the
try/block-wrapper
branch
from
8d3b156
to
9d23cb0
Compare
ellatrix
force-pushed
the
try/inner-block-wrapper
branch
from
478ce5e
to
6799502
Compare
|
Size Change: -395 B (0%) Total Size: 865 kB
ℹ️ View Unchanged
|
ellatrix
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
ellatrix
force-pushed
the
try/inner-block-wrapper
branch
from
6cfdb39
to
94d8ca7
Compare
| '__experimentalMoverDirection', | ||
| 'allowedBlocks', | ||
| 'template', | ||
| ] ) } |
There was a problem hiding this comment.
This component could benefit from a hooks rewrite, which will remove the need to omit props used by this component. I'll consider it out of scope for this PR though.
There was a problem hiding this comment.
So just to clarify here, the extra ...props are needed for the "style" in this PR right?
There was a problem hiding this comment.
That's correct. Any remaining props on InnerBlocks should be passed down to the block wrapper.
|
Fixed the e2e tests and it's looking good to me now. |
ellatrix
force-pushed
the
try/inner-block-wrapper
branch
from
1fea682
to
5b0cdc0
Compare
|
This is working very well. Let's merge and iterate. |
| export function hasInnerBlocksContext( element ) { | ||
| return !! element.querySelector( '.block-editor-block-list__layout' ); | ||
| return ( | ||
| element.classList.contains( 'block-editor-block-list__layout' ) || | ||
| !! element.querySelector( '.block-editor-block-list__layout' ) | ||
| ); |
There was a problem hiding this comment.
There are tests for this function. Probably should have been updated.
| : () => <InnerBlocks.ButtonBlockAppender /> | ||
| } | ||
| __experimentalTagName={ Block.div } |
There was a problem hiding this comment.
It wasn't introduced here, but it's not clear to me why we limit to a predefined set of tag names, vs. some sort of factor function (createBlockComponent( tagName )) or just rendering some element like <BlockComponent tagName="div" /> here. What if I wanted a section block?
There was a problem hiding this comment.
It's still experimental, but the intention is to add all tags.
| 'data-has-explicit-width': true, | ||
| }; | ||
| } | ||
| lightBlockWrapper: true, |
There was a problem hiding this comment.
Is this experimental? Where do we document it?
There was a problem hiding this comment.
Ah, it should have been prefixed as well with __experimental.
| &[style] { | ||
| &[style*="flex-basis"] { |
There was a problem hiding this comment.
Are we now applying other styles? This was already pretty fragile in the first place, but was a compromise since existing content wouldn't have any indicators of whether a predefined width was assigned. I think if we expand this, we might want to consider deprecations to include some more durable indicator (like a data- attribute, or class name).
There was a problem hiding this comment.
Yes, there are other inline styles for block animations. Yes, would probably be best to have a separate attribute.
|
This PR had a side-effect that may have been intended, not sure. Before this PR, the last block in a nesting container would show a big visible plus button, like this:
The heuristic was a bit overthought, perhaps — it would show up at the end of a blocklist if the last block was not a paragraph. After this PR, that plus is gone. Technically, I find this to be a vastly better experience because that plus should disappear until a block is selected. However it would be good to find out if there were any ill effects from the removal of these. And it'd also be good to know exactly why it was removed — i.e. was it unintentional and is there dead code somewhere? CC: @ellatrix |
|
@jasmussen It looks like this was an accidental removal: https://github.com/WordPress/gutenberg/pull/19910/files#diff-3878fabdd6add9b09483511e7ff0d9b3L64 Explicitly setting this to false won't render any appender, and undefined seems to only render it if the last block is not a paragraph. It's been set to false because, even if there was no appender, some invisible DOM would be left that messes up the :last-child rules. I didn't know something would be rendered if the last block was not a paragraph. Should it be restored? |
|
Excellent detective work. We should not restore it until we have a good plan for what to do with it. Knowing how to add it back thanks to you help, I might try my hand at a PR tomorrow. |
|
First of all I think this was a fantastic change! The side effect that I'm experiencing from this PR is that it is working as intended, but my front-end CSS for overriding column styles does not play well with the editor. This is due to how the editor handles the content width/alignments vs how my front-end works. On the front-end, there is a container element that the blocks are rendered within that has a max width and auto left/right margins, whereas the editor puts a max-width on each block and applies different margins based on the alignment. Due to my override CSS applying margins to the
I'm not sure what I'm advocating for here, because I like the idea that all blocks are rendered in this manner so that front-end styles should match 1:1... But due to the base alignment layout I wonder if at least the parent I think I may try overriding the editor default layout scheme to better match my front-end so that this isn't an issue. |
Description
closes #7770
Removes 6 wrapper
divelements from the columns and column together.I created this PR before the merge of #19701 to illustrate with the columns block how it all comes together. This PR really shows the power that blocks will have when they can entirely render themselves.
With this PR, the DOM tree of the columns block is exactly the same as the DOM tree on the front end. This removes the need add any styling for the editor. There's still a few things that ideally should be adjusted (we shouldn't be adding a margin to blocks by default because some blocks need to reset this). But you get the idea and the direction.
I believe this direction will be really important for full site editing themes and performance as well (which is built on
InnerBlocks).It will also allow us to build complex block with inner blocks that need a semantic relationship with the ancestors. Think about a list, table, or definition list built entirely with
InnerBlocks.Please note that I haven't polished the styling of the columns block yet.
Before:
After:
How has this been tested?
Screenshots
Types of changes
Checklist: