Reset margin for all children of flow layouts #35426
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
youknowriad
added
[Type] Bug
youknowriad
requested review from
ellatrix,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +23 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Thanks for tackling this! It's nice to roundout the stack rules! It mostly appears to be working exactly as it should, and a group block is a good way to test it. In the following GIF I've commented out a few extra rules I added in my own theme to complement it, but it works as it should: Two things, though. On the frontend, I'm seeing this rule correctly output: However I'm not seeing a similar rule for the first child. I would assume it to be something like this: In my theme, that would've targetted the header. But it doesn't seem to be there. The other thing, the rule appears to be output for the page title in the editor: causing it to collapse quite a bit: That last bit is not a problem, it's something I can fix in an editor style if need be. But judging by the code changes it doesn't seem to have been intentional? |
|
yeah I forgot the root level, what about now? |
jasmussen
reviewed
Oct 7, 2021
There was a problem hiding this comment.
Nice, works well for me now, and feels good to land since it's only the missing half of the full stack CSS equation. So I'm tempted to just give this the green check.
But I'm still not sure why it targets the site title wrapper (.edit-post-visual-editor__post-title-wrapper > *) — it appears to do it in my theme, where I leverage the gap property, and it's causing the title block to have zero margin:
It's trivial for me to add that back in my editor style, but it seems like a bug it's there in the first place.
|
Worth noting, I'm not seeing the title margin cancelled out on either Empty Theme or TT1 blocks: I haven't yet tested TwentyTwentyTwo — how does the title block look there for you? |
|
@jasmussen It does impact the post title, that's actually the result of applying the default layout to the post title wrapper. We do this in order to apply the right "max-width" to the post title. I'm not exactly sure how to solve that at the moment but let me try a few things |
|
I'm personally okay with the trade-off because it isn't really a "block" yet, and it's one of those things that would be solved if it actually became a block. The reason I mention it is because right now it behaves add it's own wrapper with a single child, followed by the post content wrapper with all it's paragraphs. Meaning there's no gap between title and post content. |
|
Maybe a better default would be to explicitly apply the "gap" variable as a margin-top and margin-bottom of the wrapper? (I mean hard-coded in Gutenberg codebase. WDYT |
|
If the gap is leveraged by the theme, yeah that might make sense. Basically I would ideally expect the editor (post title and a bunch of paragraphs) to look the same as a H1 followed by a bunch of paragraphs on the frontend. |
|
I pushed a fix let me know how it feels. |
jasmussen
approved these changes
Oct 7, 2021
youknowriad
force-pushed
the
tweak/layout-flow-margins
branch
from
bfe053c to
c593abb
Compare
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.
closes #34441
Applies the suggestion in #34441 to reset the margin for all children of flow layouts. While the margin-top is applied to all but the first one (to space the children), the reset should apply to all children.