-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Spacer : Added color support #66433
base: trunk
Are you sure you want to change the base?
Spacer : Added color support #66433
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @benazeer-ben! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Thanks for the PR! Here I'm wondering if adding visual styles to the spacer block is contradictory to the block's purpose: If the block can have a background color then it ventures beyond its one-dimensional job of creating just "space". Rather, there are alternative container blocks that can the same thing, e.g., Group block: <!-- wp:group {"style":{"dimensions":{"minHeight":"249px"}},"backgroundColor":"luminous-vivid-orange","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-luminous-vivid-orange-background-color has-background" style="min-height:249px"></div>
<!-- /wp:group --> For that reason I'd probably skip adding any such block supports to the space block. That's just my 2c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate the great PR descriptions for all these block support PRs @benazeer-ben, thank you 🙇
This PR is testing as advertised for me. I didn't run into any issues.
✅ Global Styles apply correctly in the editor and on the frontend.
✅ Block support styles override the Global Styles as expected on both the frontend and editor
My understanding is that the spacer block was supposed to just be for layout and spacing purposes. I'm not sure it makes sense to support colors.
I don't hold that opinion strongly so if others feel it's valuable, I'd be happy to change my mind.
I've added the Needs Design Feedback
label to this one to get a design perspective.
Edit: Missed Ramon's reviewed this already 🤦
Here I'm wondering if adding visual styles to the spacer block is contradictory to the block's purpose:
That echoes my thinking just way more eloquently put 🙂
What?
Adding color support for Spacer block.
Part of #43245
Why?
Spacer block is missing color support.
How?
Adds the color block support with bg color settings via block.json
Testing Instructions
Screenshots or screencast
Backend:
Frontend: