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

Add Custom CSS support to the Group block #25413

Closed
wants to merge 2 commits into from

Conversation

youknowriad
Copy link
Contributor

This PR allows users to add custom CSS for group blocks.

  • It does so by adding a new customCSS support hook. So it can be enabled in any block pretty easily.
  • It's saved as a "css" block attribute.
  • you can use :self selector to refer to the current block.

Testing instructions

  • Add a group block
  • In the "Custom CSS textarea" in the "Advanced controls", type
:self {
    background: red;
}
  • Notice the group gets a red background on both frontend and backend.

@youknowriad youknowriad self-assigned this Sep 17, 2020
@youknowriad youknowriad added [Block] Group Affects the Group Block [Type] Feature New feature to highlight in changelogs. Needs Dev Note Requires a developer note for a major WordPress release cycle labels Sep 17, 2020
* 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using an empty style to enqueue the different block custom CSS.
Let me know if there are better ways to do that.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aristath aristath Sep 17, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@aristath aristath Sep 18, 2020

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I thought about that, the problem with that though, is that you can alter pseudo-states and things like that.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 );
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I make it experimental to start with?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@github-actions
Copy link

github-actions bot commented Sep 17, 2020

Size Change: -30.2 kB (2%)

Total Size: 1.17 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/annotations/index.js 3.52 kB +1 B
build/api-fetch/index.js 3.35 kB +19 B (0%)
build/autop/index.js 2.72 kB -1 B
build/block-directory/index.js 8.53 kB +126 B (1%)
build/block-editor/index.js 129 kB -17 B (0%)
build/block-editor/style-rtl.css 11.1 kB -2 B (0%)
build/block-editor/style.css 11.1 kB -1 B
build/block-library/editor-rtl.css 8.6 kB +11 B (0%)
build/block-library/editor.css 8.6 kB +11 B (0%)
build/block-library/index.js 135 kB +651 B (0%)
build/block-serialization-default-parser/index.js 1.78 kB -1 B
build/blocks/index.js 47.5 kB -50 B (0%)
build/components/index.js 167 kB -34 kB (20%) 🎉
build/components/style-rtl.css 15.5 kB +6 B (0%)
build/components/style.css 15.5 kB +5 B (0%)
build/compose/index.js 9.42 kB -62 B (0%)
build/core-data/index.js 12 kB +20 B (0%)
build/data/index.js 8.43 kB +4 B (0%)
build/date/index.js 31.9 kB -3 B (0%)
build/dom/index.js 4.42 kB -20 B (0%)
build/edit-navigation/index.js 10.4 kB +31 B (0%)
build/edit-post/index.js 306 kB +58 B (0%)
build/edit-post/style-rtl.css 6.24 kB +5 B (0%)
build/edit-post/style.css 6.23 kB +6 B (0%)
build/edit-site/index.js 20.2 kB +1 kB (4%)
build/edit-site/style-rtl.css 3.51 kB +374 B (10%) ⚠️
build/edit-site/style.css 3.51 kB +374 B (10%) ⚠️
build/edit-widgets/index.js 17.5 kB +744 B (4%)
build/edit-widgets/style-rtl.css 2.8 kB +44 B (1%)
build/edit-widgets/style.css 2.8 kB +44 B (1%)
build/editor/index.js 45.5 kB +354 B (0%)
build/editor/style-rtl.css 3.8 kB +2 B (0%)
build/editor/style.css 3.8 kB +2 B (0%)
build/escape-html/index.js 733 B -1 B
build/format-library/index.js 7.49 kB -14 B (0%)
build/html-entities/index.js 621 B -1 B
build/i18n/index.js 3.55 kB +1 B
build/is-shallow-equal/index.js 709 B -1 B
build/list-reusable-blocks/index.js 3.02 kB +1 B
build/media-utils/index.js 5.12 kB +21 B (0%)
build/notices/index.js 1.69 kB +1 B
build/nux/index.js 3.27 kB +17 B (0%)
build/plugins/index.js 2.44 kB +4 B (0%)
build/priority-queue/index.js 789 B -1 B
build/redux-routine/index.js 2.85 kB +1 B
build/server-side-render/index.js 2.61 kB +5 B (0%)
build/shortcode/index.js 1.7 kB -1 B
build/url/index.js 4.06 kB -3 B (0%)
build/viewport/index.js 1.74 kB -1 B
build/warning/index.js 1.13 kB -1 B
ℹ️ View Unchanged
Filename Size Change
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/style-rtl.css 7.6 kB 0 B
build/block-library/style.css 7.59 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/data-controls/index.js 1.27 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/element/index.js 4.45 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.34 kB 0 B
build/rich-text/index.js 13.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@@ -0,0 +1,72 @@
<?php
/**
* Align block support flag.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Align block support flag.
* Custom-CSS block support flag.

@jasmussen
Copy link
Contributor

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.

@aristath
Copy link
Member

This is indeed very exciting... And would also allow for the things outlined in #24577

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Overall I like this direction a lot, even if we need to polish part of the implementation.

$has_custom_css_support = false;
if ( property_exists( $block_type, 'supports' ) ) {
$has_custom_css_support = gutenberg_experimental_get( $block_type->supports, array( 'customCSS' ), false );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Comment on lines +47 to +50
if (
! ( $block_type && $block_type->render_callback ) &&
! $has_custom_css_support
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@ZebulanStanphill
Copy link
Member

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 <style> tag.

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.

@youknowriad
Copy link
Contributor Author

@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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +66 to +68
node.innerHTML = replace(
attributes.css,
/:self/g,
Copy link
Contributor

Choose a reason for hiding this comment

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

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 );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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. :)

@cguntur cguntur mentioned this pull request Oct 14, 2020
17 tasks
@oandregal oandregal mentioned this pull request Nov 18, 2020
82 tasks
Base automatically changed from master to trunk March 1, 2021 15:44
@youknowriad
Copy link
Contributor Author

This is becoming oldish, if folks want to refresh this, go for it, I'm focusing on other PRs at the moment.

@youknowriad youknowriad deleted the add/group-custom-css-support branch July 12, 2021 16:46
@aurooba
Copy link
Member

aurooba commented Jan 26, 2022

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.

@youknowriad
Copy link
Contributor Author

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.

@mtias
Copy link
Member

mtias commented Jan 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants