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

StyleBook: Make available in all classic themes #67782

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

t-hamano
Copy link
Contributor

Follow-up #66851
See this discussion: #66851 (comment)

What?

This PR enables the stylebook regardless of whether a classic theme has theme.json or supports editor styles.

Why?

I've noticed that editor-related styles can be added via code like this for example, which is not related to whether you have a theme.json or not, and whether it supports editor styles or not:

function test_enqueue_block_editor_assets() {
	wp_enqueue_style(
		'emptyhybrid-test',
		get_theme_file_uri( '/style.css' ),
	);
}
add_action( 'enqueue_block_assets', 'test_enqueue_block_editor_assets' );

// This hook really shouldn't be used for block-related styles, but there may still be themes that use it this way.
function test_enqueue_block_editor_assets() {
	wp_enqueue_style(
		'emptyhybrid-test',
		get_theme_file_uri( '/style.css' ),
	);
}
add_action( 'enqueue_block_editor_assets', 'test_enqueue_block_editor_assets' );

It probably makes more sense to cover all the possibilities and make it available to all classic themes, rather than adding complex conditional statements.

Testing Instructions

  • Access http://localhost:8889/wp-admin/.
  • Activate the Emptyhybrid theme.
  • Make sure Appearance > Design is available.

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. [Feature] Style Book labels Dec 10, 2024
@t-hamano t-hamano self-assigned this Dec 10, 2024
@t-hamano t-hamano marked this pull request as ready for review December 10, 2024 08:36
Copy link

github-actions bot commented Dec 10, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: jasmussen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@@ -130,7 +130,7 @@ function gutenberg_styles_wp_die_handler( $default_handler ) {
* @global array $submenu
*/
function gutenberg_add_styles_submenu_item() {
if ( ! wp_is_block_theme() && ( current_theme_supports( 'editor-styles' ) || wp_theme_has_theme_json() ) ) {
if ( ! wp_is_block_theme() ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'll leave @tellthemachines to chime in with a more educated review, but I was just wondering what the implication of this comment from #66851 (comment) would be:

because if the theme has neither, the style book will be useless.

Does these mean if a theme doesn't have editor or theme.json styles, the style book would just render the default styles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does these mean if a theme doesn't have editor or theme.json styles, the style book would just render the default styles?

That's what I thought at first too. But as mentioned in this PR, it seems there's a way to apply styles for blocks on the editor side other than theme.json or editor-styles support.

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 not convinced we should do this, as having a condition means themes have the choice to opt in or out. If themes really want the stylebook, it costs nothing to add theme support for editor-styles. Twenty Twenty has it even though it enqueues its editor styles via enqueue_block_assets as in your example.

Copy link

Flaky tests detected in 6e47b6c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12252303353
📝 Reported issues:

@t-hamano
Copy link
Contributor Author

Thanks for the reviews, everyone!

First, should we discuss whether / how to provide a way to opt in or out of the StyleBook?

Personally, I feel that it's best to enable the StyleBook for all classic themes and that the opt-in/opt-out feature itself is unnecessary. Here's why:

  • There are various ways to add block-related styles to the editor. theme.json, editor-styles support, enqueue_block_assets hook, enqueue_block_editor_assets hook, etc. Not limited to themes, plugins may add editor styles via hooks. Or you may have some custom blocks that enqueue editor styles via block.json. Given all of this, it seems difficult to accurately determine whether block-related styles are registered in the editor.
  • I think it's useful to be able to see the default styles for blocks via the StyleBook, even if you don't have any block-related styles in the editor. Because the default styles are where you can see how Gutenberg applies styles to core blocks, and those block styles are what actually get displayed on the frontend.

What do you think?

cc @jasmussen @ndiego @bph

@jasmussen
Copy link
Contributor

Thanks for the PR.

I think it's useful to be able to see the default styles for blocks via the StyleBook, even if you don't have any block-related styles in the editor. Because the default styles are where you can see how Gutenberg applies styles to core blocks, and those block styles are what actually get displayed on the frontend.

This is a key discussion to have, and conceptually feels valid enough, especially for WordPress users that know what they are doing. These might already know what the customizer is, what the site editor is, what widgets are, and what the role of each of those is. But for someone entirely new, coming to a section called "Style book" and being able to do nothing at all, I just have a feeling that will be confusing. Even if we added explanatory text in the sidebar, most people in my experience, don't "read the manual" unless they have to. And even if they did, they might ask, "what's the point then?"

This sounds like a strong opinion, it's not, and I'm happy to defer. But one of the opportunities we have now, is testing this in the plugin. The outreach group has already been pinged on the other PR, so hopefully this will be tested in the plugin and we'll have some broader feedback. Notably, the plugin also runs on WordPress.com, where support may hopefully gather some input. CC: @annezazu for awareness.

All that is to say, it may be good to let this one sit for a bit, while we wait for what's already shipping to gather a little feedback, after which point we may be better equipped to decide. What do you think?

Without data, however, my instinct would still go towards: if you allow block styles to be editable, you get the style book. Immediately this would make the point of the style book clear and it would make the tool useful. Could this happen without opt-in? Why not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Style Book [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants