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

Post Title block - add spacing.margin support #35684

Closed
wants to merge 6 commits into from
Closed

Post Title block - add spacing.margin support #35684

wants to merge 6 commits into from

Conversation

colorful-tones
Copy link
Member

Description

Addresses #35522

How has this been tested?

Tested and verified changes locally with TwentyTwentyTwo theme enabled.

Screenshots

Screen Shot 2021-10-15 at 12 31 11 PM

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • n/a My code follows the accessibility standards.
  • n/a I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • n/a I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@kjellr
Copy link
Contributor

kjellr commented Oct 15, 2021

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:

  • Margin entered in block markup for the post title works in the editor at all times: Even if support has not been declared here or in the theme.
  • Margin entered in block markup only works in the front-end if its specifically declared in block.json (as you've done in this PR).
  • The UI only shows up if the theme has opted in via theme.json.

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.

@colorful-tones
Copy link
Member Author

Whatever the behavior, there shouldn't be a mismatch between the front-end and the editor.

Totally agree. 💯

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.

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 theme.json/FSE.

I will monitor #28356 for further discussion. Thanks @kjellr

@kjellr
Copy link
Contributor

kjellr commented Oct 19, 2021

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.

@carolinan
Copy link
Contributor

I honestly do not understand why we want it limited to top and bottom?

@kjellr
Copy link
Contributor

kjellr commented Oct 20, 2021

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).

@kjellr
Copy link
Contributor

kjellr commented Oct 20, 2021

@colorful-tones i think we’re all set on this one now due to #35772 being merged. Thanks for getting the ball rolling!

@ndiego
Copy link
Member

ndiego commented Dec 9, 2021

Just stumbled on this PR. @colorful-tones can this be closed since #35772 is merged?

@colorful-tones colorful-tones deleted the feature/post-title-block-margin-support branch December 10, 2021 00:18
@colorful-tones
Copy link
Member Author

@ndiego yes, I just closed. Thanks for reminder! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Post Title Affects the Post Title Block [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants