Add Custom CSS support to the Group block #25413
Conversation
youknowriad
requested review from
ajitbohra,
ellatrix,
Soean,
spacedmonkey,
talldan and
TimothyBJacobs
as code owners
youknowriad
added
[Block] Group
| * This style is used to enqueue the dynamic block custom CSS styles. | ||
| */ | ||
| function gutenberg_register_custom_css_style() { | ||
| wp_register_style('wp-block-custom-css', false); |
There was a problem hiding this comment.
I'm using an empty style to enqueue the different block custom CSS.
Let me know if there are better ways to do that.
There was a problem hiding this comment.
Ideally we wouldn't enqueue anything... These would be a lot better as inline styles in the element itself. 👍
If the user adds styles to a specific block, then they want those styles applied and they should therefore override any global CSS from stylesheets.
There was a problem hiding this comment.
Ideally we wouldn't enqueue anything... These would be a lot better as inline styles in the element itself. 👍
This assumes there's a way to alter the markup of the block and add this somewhere there. IMO it's a bit more fragile because of the DOM changes needed.
There was a problem hiding this comment.
I was thinking we could handle this the same way we handle extra, user-defined classes... The logic we follow for that is pretty robust and we could do the same for custom-css
There was a problem hiding this comment.
We don't have extra markup inside the block wrapper though, also, we can't assume the wrapper accept children or is a self closing tag.
There was a problem hiding this comment.
The way I was envisioning it would be a bit more restrictive...
So users wouldn't be able to type full CSS like :self{color:red;}. Instead they'd only write the rules (no selector) like color:red;.
That would then be properly added to the element as inline CSS using style="" in the wrapper itself, there's no need to alter any other markup, children etc. It would be a lot easier to sanitize too (basically esc_attr on the PHP side and an equivalent in JS).
That's why I envisioned it similar to the "Additional CSS class(es)" field... The implementation would be pretty much identical.
So the field wouldn't be so much a "custom CSS" field... it would be "custom styles" - the difference being it will only contain the actual styles so no selectors etc, can't be considered full CSS since there's nothing cascading about it.
Would also address most of @ZebulanStanphill's concerns on #25413 (comment)
There was a problem hiding this comment.
Yes, I thought about that, the problem with that though, is that you can alter pseudo-states and things like that.
There was a problem hiding this comment.
True... But if someone is knowledgeble enough to know what pseudo-elements are, they can surely add a custom class to their element and then add whatever CSS they want... They can even add CSS in a custom HTML block. 😉
A feature for custom-styles should appeal and be usable by the masses. For more advanced features there are more advanced solutions to adding CSS
There was a problem hiding this comment.
It's added to the "Advanced" section for a reason. I think that's where the disagreement lies, I do think it's a feature for advanced users regardless of how it's implemented. It's something to be used to create designs that are not possible with regular controls. Imagine creative things like animations, filters, rotations... in predefined block patterns.
You might be able to achieve these things with dedicated plugins... but for adhoc situations where you need these things, you don't want to install a plugin that supports animations, rotations for Core blocks and create potential invalidation.
There was a problem hiding this comment.
I agree with Riad that this feature is useful as it stands, even when there are other more generic tools to achieve similar results.
This feature allows styles to be tied specifically to a block, regardless of the chosen selectors. Providing :self is very useful, but it is even better when we allow advanced use cases — not just pseudo-states and nested elements — like (why not) selecting elements outside of :self.
| $has_custom_css_support = false; | ||
| if ( property_exists( $block_type, 'supports' ) ) { | ||
| $has_custom_css_support = gutenberg_experimental_get( $block_type->supports, array( 'customCSS' ), false ); | ||
| } |
There was a problem hiding this comment.
Ideally, this shouldn't be necessary here and block support application should be "opt-in" maybe? or use a dedicated function like proposed by @mcsf
There was a problem hiding this comment.
There are a few ways to interpret this, and I think you and I discussed more than one way to handle it. Can you detail your suggestions?
There was a problem hiding this comment.
I'm just saying that the function you proposed wp_block_supports should make this useless once implemented but I guess it will force us to have a render_callback for the group block.
There was a problem hiding this comment.
Thinking more here, it seems the "automatic" approach we have now make this easier, otherwise we'll have to "duplicate" the group "save" function on the backend somehow?
There was a problem hiding this comment.
I'm super undecided here. I feel like this is probably not the solution we want for the future, but I feel out of good ideas. That's also why I stressed further below that we should keep the feature experimental. That way I'm not keeping your PR from being merged. :)
| @@ -19,6 +19,7 @@ | |||
| "gradients": true, | |||
| "linkColor": true | |||
| }, | |||
| "__experimentalPadding": true | |||
| "__experimentalPadding": true, | |||
| "customCSS": true | |||
There was a problem hiding this comment.
Should I make it experimental to start with?
There was a problem hiding this comment.
Let's make it experimental. That way we have some more time to settle the matter of automatic / opt-in / opt-out solutions for front-end transformations.
|
Size Change: -30.2 kB (2%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
| @@ -0,0 +1,72 @@ | |||
| <?php | |||
| /** | |||
| * Align block support flag. | |||
There was a problem hiding this comment.
| * Align block support flag. | |
| * Custom-CSS block support flag. |
|
This is such a fascinating feature! On the one hand, this one feature can open up so much; amazingly detailed and glorious pattern designs, and so many little things you can fix when you're developing a theme and this one block doesn't yet support the feature you need. Even now, I have several specific places where I can use this feature in the FSE theme I'm tinkering on. On the other hand, it's kind of Pandoras box. I can't help but worry that now that you can fix an issue with a little CSS, you're less likely to fix it in the block itself. And that we could see a proliferation of "css hacks" that really should be fixed elsewhere. In other words I'm super torn on this one, but I can't help but feel like we should probably try it! It seems the benefits outweigh the downsides. But I'd definitely lean towards marking as experimental for now. |
|
This is indeed very exciting... And would also allow for the things outlined in #24577 |
| $has_custom_css_support = false; | ||
| if ( property_exists( $block_type, 'supports' ) ) { | ||
| $has_custom_css_support = gutenberg_experimental_get( $block_type->supports, array( 'customCSS' ), false ); | ||
| } |
There was a problem hiding this comment.
There are a few ways to interpret this, and I think you and I discussed more than one way to handle it. Can you detail your suggestions?
| if ( | ||
| ! ( $block_type && $block_type->render_callback ) && | ||
| ! $has_custom_css_support | ||
| ) { |
There was a problem hiding this comment.
Regardless of the solution in the previous comment, this piece here is definitely something to address: all the other transformations after the early return are for dynamic blocks, and gutenberg_apply_custom_css_support is the elusive exception. Static-friendly hooks and dynamic-only hooks should be invoked in clearly different places.
|
This is definitely a risky thing to add. Since you're allowing full CSS in here, one could put anything, regardless of whether or not it's targeting the current block. If someone really wants to do this, they can use the sitewide Custom CSS field and a CSS class, or they could use the Custom HTML block with a I wouldn't be opposed to adding something like this at the Global Styles level, but I don't think there's enough of a reason to add it at the individual block level. The valid use-cases are too few to outweigh the potential misuse, in my opinion. Maybe we should add a system to allow the user to create style variations instead? Maybe we should make the "Additional CSS classes" field use an interface more like the Tags interface? I don't think we should add something like this until there's a clear need for it that isn't solved by Global Styles or improvements to existing functionality. |
|
@ZebulanStanphill There are a number of designs that are very hard to achieve with just block tools. I don't think we have the capacity to add block tools for every possible design and that's where this comes in. I received a lot feedback from designers that block patterns are very limited and they'd like to be more creative for their users there. While I do agree that it's ideally addressed using block tools, solving all of these is impossible and I think for advanced users with knowledge about the pitfalls, it's completely fine. That's why it leaves in the advanced section. |
Fix to match server-side behaviour as well as user expectations.
| @@ -19,6 +19,7 @@ | |||
| "gradients": true, | |||
| "linkColor": true | |||
| }, | |||
| "__experimentalPadding": true | |||
| "__experimentalPadding": true, | |||
| "customCSS": true | |||
There was a problem hiding this comment.
Let's make it experimental. That way we have some more time to settle the matter of automatic / opt-in / opt-out solutions for front-end transformations.
| node.innerHTML = replace( | ||
| attributes.css, | ||
| /:self/g, |
There was a problem hiding this comment.
Why lodash and not String::replace? Anyway, I pushed a commit to make sure we replaced all occurrences (using the RegExp g flag).
| $has_custom_css_support = false; | ||
| if ( property_exists( $block_type, 'supports' ) ) { | ||
| $has_custom_css_support = gutenberg_experimental_get( $block_type->supports, array( 'customCSS' ), false ); | ||
| } |
There was a problem hiding this comment.
I'm super undecided here. I feel like this is probably not the solution we want for the future, but I feel out of good ideas. That's also why I stressed further below that we should keep the feature experimental. That way I'm not keeping your PR from being merged. :)
|
This is becoming oldish, if folks want to refresh this, go for it, I'm focusing on other PRs at the moment. |
|
This is a bit old, but @mtias pointed this PR out to me @youknowriad, which could be repurposed for a global styles custom CSS area, that provides the Site Editor equivalent of the Customizer Custom CSS area. What would/could be the next steps to exploring this for #30142? I'm happy to help in some way, but so far my experience has been specifically around creating blocks so I'm new to the whole Editor UI stuff. |
|
Thanks for the ping @aurooba we need to repurpose this PR basically which was very close :) The mechanism to add custom CSS should work similarly between blocks, but also any theme.json context (block type or root). once that in place, adding a UI to the global styles panel should be easy enough. |
|
Down the road, one thing that would be neat would be the ability to add custom CSS specific to a block type so you are not concerned with the selector details. |
This PR allows users to add custom CSS for group blocks.
customCSSsupport hook. So it can be enabled in any block pretty easily.:selfselector to refer to the current block.Testing instructions