Skip to content
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

Add spacing controls to all heading blocks #35772

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Oct 19, 2021

Fixes #35522.
This is a more expansive version of #35684.

When creating patterns and templates, I find myself needing padding and/or margin support most frequently on headings. In particular, they tend to require additional space above or below them so that they comfortably flow with the rest of the text. The Site Title block already has these controls, but other heading blocks do not. This fixes that by adding margin + padding controls to all blocks that generate headings:

  • Heading
  • Post Title
  • Query Title

⚠️ Note: Left/right margins may end up breaking the layout for classic themes when used on the top level of the page. We could limit to just top and bottom margin support, but since this specific issue happens when any block uses margin controls, I consider that more of a global issue and I don't think it should necessarily limit us in this specific PR. Still, we could enable just top/bottom margins if folks think that's a concern.

To test:

Try the following block markup, and note that the padding + margins appear as expected in both the front end and editor:

<!-- wp:heading {"style":{"spacing":{"padding":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"},"margin":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"}}}} -->
<h2 id="this-heading-has-margin-padding" style="margin-top:1rem;margin-right:1rem;margin-bottom:1rem;margin-left:1rem;padding-top:1rem;padding-right:1rem;padding-bottom:1rem;padding-left:1rem">This heading has margin + padding.</h2>
<!-- /wp:heading -->

<!-- wp:post-title {"style":{"spacing":{"padding":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"},"margin":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"}}}} /-->

<!-- wp:query-title {"type":"archive","style":{"spacing":{"padding":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"},"margin":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"}}}} /-->

Screenshot

Screen Shot 2021-10-19 at 12 00 12 PM

Screen Shot 2021-10-19 at 11 58 49 AM

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. [Block] Heading Affects the Headings Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Block] Post Title Affects the Post Title Block [Block] Query Title Affects the Query Title Block labels Oct 19, 2021
@kjellr kjellr self-assigned this Oct 19, 2021
Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works in for each of the three blocks enabled, in my testing.

@jffng
Copy link
Contributor

jffng commented Oct 19, 2021

Just noting for testing purposes that <!-- wp:query-title {"type":"archive","style":{"spacing":{"padding":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"},"margin":{"top":"1rem","right":"1rem","bottom":"1rem","left":"1rem"}}}} /--> needs to be added to an index or archive template inside a query.

@carolinan
Copy link
Contributor

Of the two PR options, I prefer this one.

@jasmussen
Copy link
Contributor

One problem we have is that the margin control looks identical to the padding control, and it's way too easy to confuse these two for the casual user. Apply either on a block with no background, and things will in many cases look as if they are the same. (#33221 is a good conversation about that)

Because of that problem, though, I'm uncomfortable adding both padding and margin at the same time. Could we add only margin to these, and then revisit padding in the future?

@jasmussen jasmussen mentioned this pull request Oct 20, 2021
7 tasks
@kjellr
Copy link
Contributor Author

kjellr commented Oct 20, 2021

Because of that problem, though, I'm uncomfortable adding both padding and margin at the same time. Could we add only margin to these, and then revisit padding in the future?

We could, however the site title and tagline block already have both of these controls available. So either way we're going to have to address this UI issue.

Of the two controls, I think margin is probably the more problematic one, for the reasons noted in this other issue.

I'll hold off on merge while we sort this out.

@kjellr
Copy link
Contributor Author

kjellr commented Oct 20, 2021

Chatted with @jasmussen a bit more on slack, and he pointed out that specifically for headings blocks, padding might be a weird control to have. It'll work identically to Margin in most cases. Padding generally makes more sense for blocks that could contain other blocks.

Anyway I updated the PR to only add margin support. We can always add padding later if we reconsider. It's a simple PR. 🙂

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@scruffian scruffian merged commit 1a3bda1 into trunk Oct 20, 2021
@scruffian scruffian deleted the add/spacing-support-to-all-heading-blocks branch October 20, 2021 13:52
@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block [Block] Post Title Affects the Post Title Block [Block] Query Title Affects the Query Title Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Post Title Block: Custom margin applied to Headings does is not reflected in the front-end
5 participants