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

Spacer : Added color support #66433

Closed

Conversation

benazeer-ben
Copy link
Contributor

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

  • Navigate to template/page edit.
  • Then add spacer block and apply the background color .
  • Verify that background color display correctly in both the editor and frontend.

Screenshots or screencast

Backend:
spacercolorbackend

Frontend:
spacercolorfrontend

Copy link

github-actions bot commented Oct 24, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: benazeer-ben <[email protected]>
Co-authored-by: aaronrobertshaw <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: karthick-murugan <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

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

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 24, 2024
@akasunil akasunil added [Type] Enhancement A suggestion for improvement. [Block] Spacer Affects the Spacer Block labels Oct 25, 2024
@ramonjd
Copy link
Member

ramonjd commented Nov 11, 2024

Thanks for the PR!

Here I'm wondering if adding visual styles to the spacer block is contradictory to the block's purpose:

Add white space between blocks and customize its height.

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.

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a 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 🙂

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Personally, I've never wanted to apply color to the Spacer block 😅

It would be nice to have a concrete example of why color support is needed.

@karthick-murugan
Copy link
Contributor

IMO, Adding color to the Spacer block helps users visually identify spacing elements within the editor. This is particularly useful for complex layouts where empty space can be difficult to discern without a visible indication.

@aaronrobertshaw
Copy link
Contributor

IMO, Adding color to the Spacer block helps users visually identify spacing elements within the editor. This is particularly useful for complex layouts where empty space can be difficult to discern without a visible indication.

The adoption of color supports means that these "spacing elements" cease to be just that. They become visual elements as the colors get applied. Those colors are then visible not just to the user editing layouts but also the site's visitors on the frontend.

There are a few tools at the user's disposal to identify spacing elements e.g. on hover outlines, list view sidebar etc.

The proposed use case might be better addressed through its own dedicated issue. The color block supports aren't a suitable solution to that problem.

My vote would be to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Spacer Affects the Spacer Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants