Use color panel for contentOnly pattern editing #71982
Conversation
jeryj
added
[Feature] Write mode
[Type] Enhancement
|
Size Change: +254 B (+0.01%) Total Size: 2.45 MB
ℹ️ View Unchanged
|
|
This is looking good. I think it would be even better if we could add all the elements in - that would also meant that as we add more elements, they would also get added here. |
packages/block-editor/src/components/inspector-controls-tabs/styles-tab.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in ffa1529. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19131529412
|
jeryj
force-pushed
the
try/content-only-color
branch
from
b5bbcd5 to
ebdee8a
Compare
| }, | ||
| }; | ||
| }, | ||
| [ elements ] |
|
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. |
| "textColor": "luminous-vivid-orange", | ||
| "className": "wp-block-comments-query-loop comments-post-extra" |
There was a problem hiding this comment.
These values all match, but are reordered. I'm not sure what caused this to be different.
|
Thanks for trying this out! I'm really not sure about the feature though. The reason we have color palettes and block style variations is precisely to avoid adding random colors that might clash with the rest of the website 😅 With the improvements to editing the pattern design in #72044, I hope we can do without this altogether! But if not, then at a minimum we should ship this with a way for it to be disabled at the theme level. I'm mostly thinking of agencies and their need to lock things down as much as possible. |
| let foundButton = false; | ||
| let foundHeading = false; | ||
|
|
||
| for ( const contentClientId of contentClientIds ) { |
There was a problem hiding this comment.
Not sure if this is faster or slower, but an alternative could be
const blockNames =
select( blockEditorStore ).getBlockNamesByClientId(
contentClientIds
);
return {
hasButton: blockNames.includes( 'core/button' ),
hasHeading: blockNames.includes( 'core/heading' ),
};
There was a problem hiding this comment.
Yours is way cleaner. I doubt there's much perf difference. Thanks for the refactor!
| const settings = useBlockSettings( blockName ); | ||
| const { updateBlockAttributes } = useDispatch( blockEditorStore ); | ||
|
|
||
| const { hasButton, hasHeading } = useSelect( |
There was a problem hiding this comment.
Nice touch. I was initially thinking to just show a defined subset, but it's fancier to detect the block!
| import { ColorEdit } from '../../hooks/color'; | ||
| import { ColorToolsPanel } from '../global-styles/color-panel'; | ||
|
|
||
| function SectionBlockControls( { blockName, clientId, contentClientIds } ) { |
There was a problem hiding this comment.
Seems fine to abstract these and not fill the hooks with too much section-specific logic. If more styles are required down the track we might revisit.
There was a problem hiding this comment.
Agreed. They were easily accessible one level up, so I thought passing them in would be easier for now.
@tellthemachines - Agencies will definitely want to exclude this 😅 But I think most people want to customize colors from time to time, even if it is garish :)
My thought is that we don't want them to exit to full design capabilities unless necessary. I think having the color will save a lot of people from needing to go into the full design capabilities at all. But, I'm happy to defer to others on this or do user testing. |
Good point. How could we do this at the pattern level? I see there's a top-level "patterns" object in theme JSON - maybe a prop there. Identifying a pattern would probably be therefore more important in order to turn it off then? This is relevant WordPress/wordpress-develop#10180 |
Would we need to do it at the pattern level? I'm thinking of theme level because it's more likely website builders/agencies would want to disable it wholesale for a website. |
Sorry, that was a typo - you're right, I meant theme level. 🤦🏻 My brain was caught between that idea, and how to identify a pattern from within a template, which is what WordPress/wordpress-develop#10180 experiments with. I'm guessing the latter is required so that all patterns are treated consistently. |
|
I think we should only land this if we expect it to be the default content only experience to cover the 80% use case. Maybe this is the default experience for a content-only unsynced pattern, while a synced pattern with content overrides does not include it? I think Agencies prefer synced patterns that can be updated across the whole site, and enabling overrides for the pieces they want changed. I agree that color options there would not be a good idea. For a general user inserting default core unsynced patterns from the inserter, I think they will likely want color options to set that one pattern they want without needing to understand synced with content overrides vs unsynced patterns oh-my 😅 How does removing color options for synced patterns sound? |
Synced patterns aren't content only anyway at the moment, so this doesn't matter |
jeryj
force-pushed
the
try/content-only-color
branch
from
3168b09 to
4536012
Compare
| } | ||
|
|
||
| if ( isSectionBlock ? hasBlockStyles : hasStyleFills ) { | ||
| if ( hasBlockStyles || hasStyleFills ) { |
There was a problem hiding this comment.
Some section blocks (cover block) don't have block styles, so if ( isSectionBlock ? hasBlockStyles : hasStyleFills ) was returning false for cover blocks because it does not have any block styles. To add the color fills check for cover blocks, it would then look like if ( isSectionBlock ? ( hasBlockStyles || hasStyleFills ) : hasStyleFills ), which was redundant. So I simplified it down to if ( hasBlockStyles || hasStyleFills )
There was a problem hiding this comment.
I was just testing with posts/query blocks. They contain a no results block with a paragraph inside. When I click on the paragraph, the colors tab appears because the hasStyleFills test is satisfied.
I wonder if we need an extra, special exception for the experiment, e.g., if ( isContentOnlyExperiment && hasBlockStyles && hasStyleFills )
Kapture.2025-10-20.at.10.46.14.mp4
I actually don't know! Just asking 😄
There was a problem hiding this comment.
I think the styles tab should always appear so maybe the condition should just be if ( isContentOnlyExperiment )
|
A few issues I noticed when testing:
|
scruffian
force-pushed
the
try/content-only-color
branch
from
3bd6087 to
9ec4d81
Compare
This is expected
This is not the case - what I thought were buttons were actually headings!
I fixed this. |
|
I think this PR is looking good. I wonder if it's worth splitting it up a bit into:
|
| ); | ||
| return { | ||
| hasButton: blockNames.includes( 'core/button' ), | ||
| hasHeading: blockNames.includes( 'core/heading' ), |
There was a problem hiding this comment.
What do you think about removing the control and the style when the element is deleted?
From what I can see, if the element is deleted inside the pattern, the style control persists when a value is set.
Kapture.2025-10-24.at.11.02.48.mp4
I'm not sure it's a huge problem, so more of a UX question.
There was a problem hiding this comment.
I'm not sure we should be allowing deletion of elements in contentOnly!
There was a problem hiding this comment.
I'm not sure we should be allowing deletion of elements in contentOnly!
My understanding was also that things wouldn't be able to be deleted.
if the element is deleted inside the pattern, the style control persists when a value is set.
Either way, if we can delete, we should be able to undo to add it back. If we remove the set value, then they undo the change, I'd want the style to still be applied. So, I think leaving the value set is fine rather than removing it in this case.
We could extract the part about exporting the ColorEdit panel, but we'd only want to do that if we plan to use it somewhere else. If we don't ship the content only pattern thing, then the ColorEdit could still be exported as this PR uses it, but it wouldn't really offer any benefit. As a result, I think we should leave it bundled in this PR. Is there anything else we should do to this PR, or is it ready for approval? |
| </PanelBody> | ||
| </div> | ||
| ) } | ||
| { isSectionBlock && ( |
There was a problem hiding this comment.
Things we would do even if we remove the experiment
Is it worth putting this behind the experiment? Or are the checks in isSectionBlock enough?
There was a problem hiding this comment.
I added a check, just in case :)
Co-Authored-By: Ramon <ramonjd@gmail.com>
…en if they don't have a styles pane
scruffian
force-pushed
the
try/content-only-color
branch
from
506cd6e to
e864e25
Compare
scruffian
left a comment
scruffian
left a comment
There was a problem hiding this comment.
Looking good thank you. Let's bring it in!
Fixes #72018
What?
Try adding a color selection option of the top level item in the pattern selector.
Why?
Seeing if we can reduce the need to exit content only pattern editing by exposing the top level group colors.
How?
Export the ColorToolsPanel so we can use it on a parent group block even though an inner block is selected. I tried to do the minimum untangling to get it to do this.
It uses the group clientId, then checks the blocks within it to see if there are heading or button blocks. If there are, it also offers those color selectors.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Recording.2025-10-10.at.1.59.46.PM.mov