Try: Refactor appender margin. #33088
Merged
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
jasmussen
added
[Type] Bug
7 tasks
|
Size Change: +739 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
getdave
approved these changes
Jun 30, 2021
There was a problem hiding this comment.
Thanks for picking this up and using a most resilient fix 🎖️
In my testing I got the following results:
✅ Widgets appenders
✅ Columns appenders
✅ Group appenders
✅ Buttons appenders
✅ Social appenders
✅ Nav appenders
✅ Nested variants of the above
I think this is good to go 🚢
packages/block-editor/src/components/block-list-appender/style.scss
Outdated
Show resolved
Hide resolved
jasmussen
force-pushed
the
try/left-margin-appender-fix
branch
from
6e25eb2
to
86364cc
Compare
|
I forced in the naive hope that would cause the buttons validation test to pass, but I don't think it's been fixed in trunk yet, so this one will likely end up red again. |
kevin940726
reviewed
Jun 30, 2021
| } | ||
|
|
||
| // Cancel any left margin if the black plus is the only child. | ||
| &:first-of-type .block-list-appender__toggle { |
There was a problem hiding this comment.
I don't really understand this part, I might be missing some context. Why will it be the only child if it matches :first-of-type?
There was a problem hiding this comment.
Excellent question, thanks for calling it out.
first-of-type is used because the appender is not the first-child in an otherwise "empty" navigation block:
Shown here with zero width is an actual placeholder state for the navigation block. That placeholder state exists here but has zero with so that it can ensure the height of the navigation block is the same between the empty state, and when there are menu items inside, ensuring no layout shift when you insert.
Observe here in the top right corner that the margin-left: 0; rule is correctly applied to the appender when the first-of-type rule hits the target:
Here, it's no longer the first-of-type, after a menu item has been inserted, and the 8px left margin applies:
That same rule applies here as well:
As noted in #33087 (comment), I think ultimately we need to remove the black plus from the editing canvas entirely, make it a popover that exists outside the canvas/iframe and is positioned in a way that doesn't cause layout shifts. That will be an opportunity to tear out all these complex rules.
In the mean time, I will add a comment to elaborate on why first-of-type is used. Sound good?
tellthemachines
added
the
Backport to WP 6.7 Beta/RC
|
@jasmussen We should include this in Gutenberg 11.0.0 release. Could you ping me when merged? Much appreciated. |
|
Thanks everyone! |
getdave
pushed a commit
that referenced
this pull request
Jul 1, 2021
* Try: Refactor appender margin. * Expand comment. * Add clarifying comment.
youknowriad
pushed a commit
that referenced
this pull request
Jul 5, 2021
* Try: Refactor appender margin. * Expand comment. * Add clarifying comment.
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Jul 6, 2021
* Try: Refactor appender margin. * Expand comment. * Add clarifying comment.
7 tasks
7 tasks
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
Fixes #33085. Alternative to #33087.
As detailed in #33087 (comment), there's a small explicit left margin on the black appender plus, and an intentional right auto margin, so as to work inside flex containers:
It has to have an explicit left margin that isn't "auto", in order to work inside flex containers:
The problem as reported is that this left margin is applied also to the big white square plus, the "button" appender, and it shouldn't be.
This PR tries to move the margin from the general block appender itself (both black and white), to the specific button type inside (either black or white), thus solving the problem.
How has this been tested?
But it's also important to test this doesn't regress any of the other appenders in any of the contexts.
Checklist:
*.native.jsfiles for terms that need renaming or removal).