Lighter block DOM: remove extra div wrapper#19010
Conversation
|
This is impressive work. Thank you for doing this. For every div you remove, the closer we get to being able to make the table block use InnerBlocks :) At a glance, this looks to behave the same as master does, even when it comes to floats: I did notice an issue with embeds that have long captions, but this issue is present in master also, so not an issue with this branch: However it seems like drag and drop broke. Master: This branch: |
|
Looks like the main difficulty here is the block movers, so we should either wait for them to be popovered or for them to move to the contextual toolbar. |
454f1d8 to
fc7f65a
Compare
fc7f65a to
24ac252
Compare
|
The previous issues are now fixed! |
|
Very excited for this PR! Great work. I'm seeing a few small things: Here's a cover image with a background: Master: It looks as though in this branch, the block borders are closer to the content itself (the text is centered). Reusable blocks inside a cover seem to confirm that: The issue looks to be the same inside columns: So it looks like the remaining issue is related the the postion of block active borders in nested blocks. It sure would make things easier if the efforts in #18667 were closer, but until they are, I think we have to be careful to keep the current experience as is. |
24ac252 to
4df9d1e
Compare
|
@jasmussen It looks like the lines I removed in 4df9d1e were causing the issue. These CSS rules seem outdated, as it mentions a hover line and setting menu. Do you think they are safe to remove? They are only applied to root blocks, so it would be good to make the experience more consistent. |
As a quick sanity check, yes, the branch now appears to work as it should! Nice work. To your question — yes those rules are outdated and needs a fresh look. Can we remove them? It looks like things are working fine without them, so probably yes. Back in earlier versions, the mover control on the left would appear on hover, even on an unselected block. The same would happen for the settings menu, the ellipsis button, which back then was not part of the block toolbar. In order to make these accessible when just on hovering the block, the block had to be widerto accommodate this. Since both of those controls are now available on click instead, this is no longer an issue. I suppose the only thing to look for is the blue line that indicates where a block will be inserted when hovering in the block library: And this blue line could use some love, it doesn't appear to work with nested blocks. But this is a non urgent separate issue that is not the result of this branch. |
|
@jasmussen Ah, that blue line... 😅 I'll have a thorough look at that sometime soon. It's a bit related to the sibling/in-between inserters as it should have the same position. This is all certainly on my to-do list. |
|
Don't take too much on your todo list! It's something we'll get to! |
| // Center the block toolbar on wide and full-wide blocks. | ||
| // Use specific selector to not affect nested block toolbars. | ||
| &[data-align="wide"] > .block-editor-block-list__block-edit > .block-editor-block-contextual-toolbar, | ||
| &[data-align="full"] > .block-editor-block-list__block-edit > .block-editor-block-contextual-toolbar { |
There was a problem hiding this comment.
These rule are dead since no contextual toolbar is rendered inside a block.
|
The branch should now be good. I tested all updated blocks, all blocks states and alignments. |
jasmussen
left a comment
There was a problem hiding this comment.
Testing this, I can't find issues anymore where borders are not correctly aligned, or where the block toolbar is not properly aligned.
CSS wise, this is an impressive refactor, and the reduced DOM nodes is going to be very welcome.
I would appreciate if this PR could get a sanity check from another, but otherwise it tests well for me.
|
I tried out the branch and everything seems to be working fine. 🙂 |
|
Thanks for the reviews and help! |
|
I guess this could have some impact (probably good impact) on theming Gutenberg. I feel it deserves a dev note? |
|
Definitely. Hopefully by that time there's more improvements to talk about as a whole. |
|
A beginning draft of a dev note, which I expect you all to discard or correct 😄
|













Description
This PR removes an unnecessary
divelement wrapper from the block DOM, and simplifies the CSS selectors. Currently there are threedivwrapper elements around blocks in master. This PR reduces it to two (outer and inner).The main change is that the block border is now set with
:beforeon the outer block wrapper instead of the removed middle wrapper.It's easiest to review this PR when hiding whitespace changes: https://github.com/WordPress/gutenberg/pull/19010/files?utf8=✓&diff=unified&w=1
How has this been tested?
All block styles need to remain the same.
Screenshots
Types of changes
Refactor
Checklist: