-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Gallery: register styles with style engine #43070
Gallery: register styles with style engine #43070
Conversation
), | ||
); | ||
|
||
gutenberg_style_engine_get_stylesheet_from_css_rules( |
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 think we might have to come up with a way to ensure these styles are printed after the layout styles:
See:
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.
Following the testing in #41423, it looks like the fallback styling rules aren't being output on a classic theme for me (TwentyTwentyOne), so if I add the following to my theme's css, it doesn't affect the gallery gap:
html {
--wp--style--gallery-gap-default: 37px;
}
From a bit of introspection, it appears that the fallback rules in $fallback_gap
are being stripped in WP 6.0, as it's a pretty complex value, and the gutenberg_safecss_filter_attr_allow_css_6_1
function here hasn't been updated to reflect the final version in 6.1 on these lines: https://github.com/WordPress/wordpress-develop/blob/2b4d385298504d412e9f0dea2e7019d53463c705/src/wp-includes/kses.php#L2508-L2512
If I copy over that preg_replace
line to the Gutenberg function, it appears that the style output works correctly, so we might need to land another backport of a backport before we can go with this PR?
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'll put up a PR to backport that line.
1fafece
to
6ae3ebe
Compare
Interesting... Seeing duplicate comments in the block supports inline CSS <style id='core-block-supports-inline-css'>
/**
* Core styles: block-supports
*/
/**
* Core styles: block-supports
*/ Might mean that the hook callback is running twice 👀 |
4beea4f
to
4a47f9e
Compare
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 keeping this one current @ramonjd! It's mostly working, but I ran into an issue with fallback gap on classic themes. I think it's uncovered a missing backport of allowed css functions in Gutenberg, but I've left a comment where we'll likely need to follow-up there.
* @param string $style String containing the CSS styles to be added. | ||
* @param int $priority To set the priority for the add_action. | ||
*/ | ||
function gutenberg_enqueue_block_support_styles( $style, $priority = 10 ) { | ||
_deprecated_function( __FUNCTION__, '6.1' ); |
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.
Should we remove this deprecation notice since it didn't make it into 6.1, and instead add it back in once we're ready to start adding in 6.2
versions of these functions?
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.
Oh yeah 🤦 This PR is old.
Will do, thank you!
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 like there's a wordpress-6.2/script-loader.php file now. I could move it there and keep the notice, and bump it to 6.2
), | ||
); | ||
|
||
gutenberg_style_engine_get_stylesheet_from_css_rules( |
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.
Following the testing in #41423, it looks like the fallback styling rules aren't being output on a classic theme for me (TwentyTwentyOne), so if I add the following to my theme's css, it doesn't affect the gallery gap:
html {
--wp--style--gallery-gap-default: 37px;
}
From a bit of introspection, it appears that the fallback rules in $fallback_gap
are being stripped in WP 6.0, as it's a pretty complex value, and the gutenberg_safecss_filter_attr_allow_css_6_1
function here hasn't been updated to reflect the final version in 6.1 on these lines: https://github.com/WordPress/wordpress-develop/blob/2b4d385298504d412e9f0dea2e7019d53463c705/src/wp-includes/kses.php#L2508-L2512
If I copy over that preg_replace
line to the Gutenberg function, it appears that the style output works correctly, so we might need to land another backport of a backport before we can go with this PR?
Great sleuthing, thanks @andrewserong I'll do the backport in another PR since it's bit separate and come back and test the scenario. I appreciate the test! 🙇 |
Oh, thanks! I've opened up a backport PR already in #44962 — I mightn't get a chance to test it properly today (I just copy + pasted and hoped for the best for the moment), so might leave it until next week to try to get it merged, if it isn't too urgent |
90f577b
to
f5e9f2a
Compare
Merged! Thanks a lot for the quick PR |
Registering gallery styles with style engine
…er.php, so we can formally deprecate the function in 6.2
f5e9f2a
to
84d1e7b
Compare
The styles seemed to work ok for me on Classic themes. I think #41015 removed the need to worry about the priority. This tested pretty well for me. The only thing I noticed is that setting the gap via either the deprecated |
Thanks for testing @glendaviesnz I activated 2021 theme, then added the following to :root {
--wp--style--gallery-gap-default: 22px;
--gallery-block--gutter-size:11px;
} This indeed worked for the frontend, but not the editor. I had to add the CSS to Is that expected. Which classic theme did you test with? |
Given that the likes of TwentyTwentyOne have lots of CSS vars in ./assets/css/style-editor.css I am picking we can assume it is normal to have to do that to get any extra vars to work in the editor, so let's leave it as that for now. |
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.
This tested well for me with block-based and classic themes.
@ramonjd, is there a separate core ticket for deprecating |
@Mamaduka @glendaviesnz I've opened up a trac ticket to flag the deprecation of the function: https://core.trac.wordpress.org/ticket/57647 The short version is: as of 6.2 all usage of As far as I can tell, we can't deprecate the function until after the JS packages update has happened, as that change will include the switch in the gallery block's |
Update: I've opened up a PR to deprecate |
Depends on:
What?
Update gallery to use style engine enqueuing, and also
WP_HTML_Tag_Processor
to add the unique class.Why?
gutenberg_enqueue_block_support_styles()
will soon be deprecated.Testing Instructions
Since https://core.trac.wordpress.org/ticket/56353 WordPress supports CSS custom vars. It will be shipped with 6.1.
For sites running < 6.1 Gutenberg allows specific CSS properties only. See: #43071
Test in classic and block themes.