-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Post Title block - add spacing.margin support #35684
Post Title block - add spacing.margin support #35684
Conversation
Thanks @colorful-tones! This works as expected, and it gets the margin working on the front-end. That said, from my tests around margin support it appears:
This seems wrong? Whatever the behavior, there shouldn't be a mismatch between the front-end and the editor. I think this PR seems fine, but that bug still exists either way. I also think it's going to be tedious to enable this on a per-block basis. Margin feels like the sort of control that should be enabled globally, even if the UI isn't shown per-block. So I think we should hold off and discuss that for a moment in #28356 before we merge. |
Totally agree. 💯
Again, agree. I really like the idea of having preset options for margin/padding, see: Add a selection of preset spacing values to supplement/replace custom padding/margin options #35306. Similar to Font Size: Small, Medium, Large, Extra-Large. I feel like these values would be a good first step for theme authors and users. Then, a Phase II and further down the line we add ability for users to place custom Margin/Padding (per position: top, left, right, bottom) and incorporate that in to I will monitor #28356 for further discussion. Thanks @kjellr |
I opened a more expansive version of this PR so we can (hopefully) get this working more consistently across all heading blocks: #35772 I also opened WordPress/twentytwentytwo#135 to add custom margin support into Twenty Twenty-Two. |
I honestly do not understand why we want it limited to top and bottom? |
I think the main argument is that left/right margin will frequently break in classic themes. See the notes in #33835 (comment). |
@colorful-tones i think we’re all set on this one now due to #35772 being merged. Thanks for getting the ball rolling! |
Just stumbled on this PR. @colorful-tones can this be closed since #35772 is merged? |
@ndiego yes, I just closed. Thanks for reminder! 👍 |
Description
Addresses #35522
How has this been tested?
Tested and verified changes locally with TwentyTwentyTwo theme enabled.
Screenshots
Types of changes
General spacing margin support was rolled out in Spacing Support: Add margin block support with configurable sides #30608. Here we're adding the margin spacing support to the Post Title block.
Props to @aaronrobertshaw and @Mamaduka for guidance on getting this PR in motion
Checklist:
*.native.js
files for terms that need renaming or removal).