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

Avatar: add padding support #43519

Merged
merged 18 commits into from
Sep 6, 2022
Merged

Avatar: add padding support #43519

merged 18 commits into from
Sep 6, 2022

Conversation

colorful-tones
Copy link
Member

@colorful-tones colorful-tones commented Aug 23, 2022

Related:

What?

Add padding support to the Avatar block.

Why?

To create consistency across blocks.

How?

Added the relevant block support in block.json

Testing Instructions

  1. Create a new post
  2. Insert a new Avatar.
  3. Adding Padding and configure.

Also, test in Global Styles as well.

  1. Go to Appearance > Editor (beta)
  2. Add an Avatar block to the template
  3. Navigate to the Global Styles and choose Avatar block
  4. Configure padding and verify

avatar-block-padding

Screen Shot 2022-08-23 at 8 11 02 AM

@colorful-tones colorful-tones added [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. [Block] Avatar Affects the Avatar Block labels Aug 23, 2022
@colorful-tones colorful-tones self-assigned this Aug 23, 2022
@ndiego
Copy link
Member

ndiego commented Aug 23, 2022

@colorful-tones I believe you will want to add box-sizing: border-box as well to this. See this comment when I added padding to the Query Title. Following the implementation in that PR should do the trick!

@colorful-tones
Copy link
Member Author

I believe you will want to add box-sizing: border-box as well to this.

@ndiego good call! I added box-sizing to style.css and I updated the gif attached in the original description.

@colorful-tones colorful-tones removed the request for review from ndiego August 23, 2022 14:44
@@ -1,4 +1,6 @@
.wp-block-avatar {
box-sizing: border-box;
Copy link
Member

Choose a reason for hiding this comment

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

This is a small nit, but can you add the comment

// This block has customizable padding, border-box makes that more predictable.

This keeps the implementation consistent with other blocks and makes it easy to understand why this has been added. Otherwise, this PR looks good to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, nothing too small, and thanks for the nitpicking! I addressed the missing comment. 👍

@colorful-tones colorful-tones requested a review from ndiego August 26, 2022 17:59
@carolinan
Copy link
Contributor

As with the site logo block, I don't think the top and bottom margins are working?

@colorful-tones
Copy link
Member Author

I don't think the top and bottom margins are working?

@carolinan From all of my tests margin and padding on all axes are working.

Here is today's test

avatar-padding-margin-support.mp4

Here is my testing environment:

  • WordPress 6.0.2
  • Twenty Twenty-Two 1.2

Copy link
Contributor

@carolinan carolinan left a comment

Choose a reason for hiding this comment

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

With margin set in theme.json only, front view, empty theme:
Screen Shot 2022-09-02 at 03 47 40

Either way, rightly, this PR touches the padding, which does work.

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one @colorful-tones, it's working nicely for me, and the padding sits nicely next to an adjacent Group block:

image

The margin issue appears to be the same one as discussed in #43520 (review) (and written up in #43404) so not a blocker for merging this one in, I don't think. Update: merged and updated the tracking issue.

LGTM! ✨

@andrewserong andrewserong merged commit 3b4526a into WordPress:trunk Sep 6, 2022
@github-actions github-actions bot added this to the Gutenberg 14.1 milestone Sep 6, 2022
@colorful-tones colorful-tones deleted the add/avatar-padding-support branch September 6, 2022 15:04
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Avatar Affects the Avatar Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants