-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Buttons block: lighten editor DOM even more. #23222
Buttons block: lighten editor DOM even more. #23222
Conversation
Size Change: -6 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
73b3f42
to
7d7e3e4
Compare
I'm seeing some broken styles with Twentytwenty. First part is before this patch, notice the spacing betwen buttons (center aligned) and how the inner button inserter is hidden when out of focus. The second part I switch to an editor after this PR where the spacing is increased and the inserter never disappears. |
7d7e3e4
to
f0aa9c4
Compare
@oxyc Thanks for catching those. The margin issue should be fixed now. The appender issue seems to be a lot more difficult to fix, however. I think the block wrapper is broken since I'm not sure how to proceed here. The Buttons block is the only core block currently using Pinging @jorgefilipecosta, since he introduced |
f0aa9c4
to
2d58fdc
Compare
AlignmentHookSettingsProvider might be better as something internal to Innerblocks using a allowedAlignments prop or something like that. I'd love for this prop to be able to define the size of the alignments though and potentially custom alignments but that's a big challenge. |
2d58fdc
to
47a2c26
Compare
47a2c26
to
ef59adb
Compare
ef59adb
to
a851725
Compare
Thinking about it some more, I'm not actually sure if the appender not disappearing is a bug or not. The behavior in this PR is consistent with other blocks like the Group block, where the appender is always visible. #24836, if merged, will change this default behavior, and perhaps that will change the behavior for the Buttons block. I think I'll wait for that PR to be merged, and see what happens when I rebase this one. |
a851725
to
de43266
Compare
I looked into everything some more, and I discovered that the appender issue was actually unrelated to Anyway, this PR actually fixed the weird invisible-appender bug, and in doing so, made the behavior consistent with the Social Icons block. But of course, the appender should simply not be there at all in the first place when these blocks (and their children) aren't selected. So I've created #25518 to fix this issue. |
de43266
to
78a08c4
Compare
packages/block-editor/src/components/block-list-appender/index.js
Outdated
Show resolved
Hide resolved
78a08c4
to
93bf03e
Compare
93bf03e
to
0935149
Compare
Alright, thanks to some fixes that landed in other PRs, this one was unblocked, and I've polished it up and rebased it. It is now ready for a final review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
width: auto; | ||
.wp-block-buttons > .wp-block { | ||
// Override editor auto block margins. | ||
margin-left: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why this changed so much though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that both the display
and width
properties were unnecessary. The former was already set by style.scss
, and the latter was already auto
by default. (And the editor never overrode it.) I tested with various alignments to make sure everything worked as expected.
I also found that the selector had a lot more specificity than it actually needed, so I simplified that. Notably, I chose to point at just the wp-block
class for the children, to make it easier for 3rd-party button blocks to be inserted and styled correctly in the Buttons block. I almost just went with a .wp-block-buttons > *
selector, but that turned out to not have enough specificity to override the margin-left
value being set by the editor in some cases.
Description
This makes the editor DOM of the Buttons block identical to the front-end (in terms of div-wrapping) by removing several wrapper divs thanks to the
__experimentalTagName
property on<InnerBlocks />
.Note that this PR does not change the React Native edit implementation.
Before:
After:
How has this been tested?
I tested to make sure Buttons blocks still looked as expected in the editor, with or without a block alignment set.
Checklist: