-
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
Post Content: Add color controls #51326
Conversation
Flaky tests detected in a21606a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5209830798
|
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.
Works as expected; thanks! 👌
screen_capture.mov
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.
Thanks for tackling this one @scruffian 👍
The Post Content block offers some edge cases that other blocks do not. I'm not 100% certain about it adopting color block supports until we resolve some of those rough edges.
The simple adoption of color supports as proposed here will expose the color controls in both the Block Inspector and Global Styles. In addition, those individual block controls won't be exposed in the Post Editor, creating further disjoint. It will be possible for a user to configure an individual Post Content block in the Site Editor and not see those styles in the Post Editor.
The global styles aspect of this PR does indeed overcome the original issue around a theme's use of per-block-type element controls. I'm just not sure that is the whole picture we need to consider.
When the sidebar tabs were introduced, there were several discussions around keeping certain blocks as settings only and using other blocks as "style providers". The primary example there was the Query block should be settings only, its inner Post Template block or a Group block could be the style providers.
In my mind, I was expecting the Post Content block to be one of these settings-only blocks given the Post Editor can't yet expose these support styles, nor the Block Inspector hide them while keeping the controls available in Global Styles.
Once we add supports, removing or relocating them in a backwards-compatible manner becomes rather difficult. How pressing is the need for this? Can we afford to invest more time and thought into available options?
Screen.Recording.2023-06-12.at.5.07.04.pm.mp4
@aaronrobertshaw thanks for the input, that's is super helpful context. The specific issue in my mind is that themes are currently using theme.json to customize the colors of this block, but users aren't able to edit it, which is leading to confusion. When you suggest the idea of "settings only", how do you see this working? Are you suggesting that the post content block would be customizable through global styles but not though the specific block instance? If so, that could definitely solve this use case. If not, then I'm curious what you mean! Thanks again for the review, super helpful as always. |
If that is the case, we should remove the possibility to change the settings from theme.json as well. |
I don't think that's necessary. It could be useful to have it editable in Global Styles but not in the specific instance. |
I'm on board with addressing that confusion. Ideally, without creating further opportunity for confusion elsewhere.
I was referring more to my expectation for the Post Content block given the known issues with it and block supports in the Post Editor. When the typography supports for the block were added there was discussion for and against proceeding with them. Ultimately, the supports got added as others merged my PR. Since then min-height support has also been added. So, I think the ship has sailed on this being a settings-only block.
I think this would be our best option to solve one source of confusion without introducing another. We can already disable the block support UIs via theme.json settings. It would be nice if we could support disabling only in one context over another. There might be some short-term options (hacks?) available if we think the Post Content block is the only one that might benefit from this. All that said, we already have other supports added to the Post Content block so it shouldn't block this PR. My apologies for the hold up 🙇 |
a21606a
to
28179f8
Compare
I rebased this, should we merge this and then follow up on disabling the supports based on context on a different PR? I'm assuming we are talking about any and all supports for the block, not just colors. |
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.
I've given this another quick smoke test and looks to still be working as discussed.
I rebased this, should we merge this and then follow up on disabling the supports based on context on a different PR?
I think so.
I'm assuming we are talking about any and all supports for the block, not just colors.
That's my understanding as well.
What?
This enables color controls for the Post Content block. Fixes #50327.
Why?
Themes often want to control these colors using Global Styles, so we should make it possible for users to change them. See #51265 for an example of why this is needed.
How?
Add the color supports to the block JSON file.
Testing Instructions