-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Design Tools: Add block instance elements color support for buttons and headings #53667
Design Tools: Add block instance elements color support for buttons and headings #53667
Conversation
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/colors.php ❔ lib/block-supports/elements.php |
Size Change: +499 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
Flaky tests detected in e36f171. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5972578569
|
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.
Just gave the style engine changes a quick smoke test — this direction is looking good to me!
@@ -58,6 +58,9 @@ final class WP_Style_Engine { | |||
'default' => 'background-color', | |||
), | |||
'path' => array( 'color', 'background' ), | |||
'css_vars' => array( |
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.
Just double-checked and refreshed my memory: the addition here and in gradient
looks safe to me and doesn't affect calls from colors.php
since in that file, we're calling gutenberg_style_engine_get_styles
with the convert_vars_to_classnames
option flag, which means that it skips adding the css_vars
in cases where we intend to grab classnames instead. For example, if we add a Post Title block to a post or page and set background / gradient colors, then with this PR applied, it'll still receive classnames and not any inline styles, which is as we'd expect 👍
E.g. this is as on trunk:
While elsewhere on the same page, the container-based element styles are applied correctly via the background-color
property, etc:
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.
The style engine™️ delivers
If more than color styles are to be supported for elements on a per block instance basis, these refactors should make that much easier.
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.
Looks and works good to me!
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.
Code-wise this is looking really good to me, too! One issue I ran into while testing, that I think is beyond the scope of this PR, is that block-level background color for a block within a Group block will not override a gradient color set further up. For example, if I set h3
in a Group block to have a gradient, then select the Heading block and set it to have a background color:
In the above screenshot, the heading has a blue gradient set at the Group block level for h3
elements, and at the individual block level, I've selected a red background color. The blue gradient wins. Setting a gradient on the child block does work, however.
I think the problem seems to be that gradient colors use the background
CSS property and that at the individual block, when we set a background color, we don't also reset background
(or background-image
). But doing that reset could be tricky to achieve in a reliable way, anyhow 🤔
In any case, other than that this PR's testing well for me, and the code looks good! Personally, I'd leave the issues with dealing with gradients and how they potentially conflict with other rules to another time/issue.
LGTM! ✨
Thanks for the reviews and testing @kevin940726 and @andrewserong 🙇
This is almost the same issue as the one I noted in the test instructions where if you set a gradient on a generic Given it's an existing issue, as it can be encountered by applying element styles for In the meantime, the worst-case scenario would be having to set a gradient that has the same color as stop points to effectively render a flat color for the background. I'll get this merged so users have the extra flexibility. This one should prove rather valuable to pattern and theme authors. |
Absolutely, it's very cool being able to use the Group block to better control the styling of child blocks in this way. Nice work getting this across the line! 🎉 |
The Group block is intended only to be an initial step, so any input on which other container classes should opt into button and heading colors is more than welcome. Column/s blocks should probably get them but things like the Media & Text or Cover blocks could also conceivably adopt them. Additionally, the long-term goal is for full support of child block-type styling, like global styles but limited to the specific section. |
Related:
What?
Allows styling button and heading elements on individual block instances.
This PR only adds button & heading color support to the Group block, other container blocks can be updated in follow-ups.
Why?
This is a small step towards allowing section specific styling based on container block instances. More background about the long term direction and goals can be found in #40318 & #48581.
How?
elements.php
to inject block instance classname if any button or heading colors have been setelements.php
to generate CSS styles for button and heading element styles and add them to the block-supports contextstyles.js
hook to generate CSS styles for any button and heading element stylesNote: The approach was refactored a little to use config arrays that might more easily be extended as we look to add further elements, e.g. caption, cite, or types of styles, e.g. typography.
Testing Instructions
:hover
h1
-h6
. This is an existing issue.Screenshots or screencast
Screen.Recording.2023-08-15.at.6.19.25.pm.mp4