Add stylebook screen for classic themes #66851
Conversation
|
Size Change: +195 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2464969. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12059752490
|
tellthemachines
added
[Type] Feature
|
Thanks for the quick turnaround. I like the implementation, especially the reuse of the stylebook component, great stuff. I know it's early days, so I only have high level thoughts that popped up while testing:
|
I just added a commit that makes sure any additional CSS from the Customizer gets loaded in both the stylebook and the editor (it probs should have been added for the editor before now) Background images are doable if we copy this logic (ideally we should extract what is reusable from there into a core function similar to What do you mean by "links" in this context? And is there a list somewhere of what else might be included in "etc."? I should add that whatever themes have provided editor stylesheets for is already included for free, e.g. TT1 with its dark mode and custom background color.
These are good ideas that would be useful for both classic and block theme users! We should explore them as next steps, given we're already having the "long list of blocks" problem with the newly moved-to-the-left stylebook view on block themes. I think eventually the black sidebar could be used as navigation not only for global styles/stylebook sections in block themes, but for stylebook navigation in classic themes too. Perhaps @WordPress/gutenberg-design folks have ideas around that? |
Some themes, e.g., Twenty Eleven allow users to edit site link colors in the Customizer.
I like the sounds of that! |
|
Nice, this is potent. The benefit of having the style book available for classic themes, is that it can become a gateway to adding theme.json properties, which on their own have the benefit of being editor-stylable. Plus, blocks also have styles in classic themes, so seeing them here, perhaps styling them here, has value.
I think there's something to think about here, as far as classic or hybrid themes often offering a lot of theme CSS, and it being confusing to see them missing here. Can we just load the theme CSS file in here, in these cases? If we did, we should probably also add a notice message somewhere that explains this. This is also important as far as setting expectations of what you can edit, thinking here of what additional site editor menu items become available. There should probably be a very strict limitation that only items defined in theme.json have counterpart menu items. I.e. if you define colors in theme.json, you get the color menu, if you define typography, you get typography (and the font library!), and so on. The interesting question becomes the "Blocks" section, which is perhaps a place to start: being able to change the default appearance of your block editor blocks would have value for any theme. But if color, typography, and spacing properties are not defined in a theme.json file, would that mean color, typography, and dimensions panels were not available for the individual blocks to style? Probably, right? Curious about your thoughts. |
tellthemachines
force-pushed
the
try/classic-stylebook
branch
from
431d0d0
to
a62465c
Compare
I'm afraid it's down to the themes to provide those styles in a way that is available to the editor. For instance, Twenty Eleven prints its custom CSS in a Basically anything that the theme defines as editor styles will also appear in the stylebook. |
I think this makes sense. Additionally, we should only show any global styles controls at all if the theme has a theme.json file. Otherwise the stylebook should remain static. This PR doesn't handle adding the global styles interface; the intention here is only to show a static stylebook with the theme styles for now. So, considering that our MVP, what needs doing to get this in shippable form?
On the last point, customizer background images aren't currently loaded in the editor, so this may be a lower priority item that could be worked on as a follow up. Is there anything else I'm missing? |
|
Ok I've updated this to remove the useless buttons and add color palette to the overview: 
If this approach looks reasonable I'll start cleaning up the code! |
|
Just took this for a quick test against the Chaplin theme (a classic theme). This theme comes with "color schemes" you can pick from in the Customizer. As I switched between different color schemes, it properly updated in the Styles section. It makes me wonder how or whether we should represent these different color schemes in the Styles interface or if it should just reflect the current settings (I'm guessing the latter for simplicity :) ): chaplin.theme.test.movVery cool to see either way! |
| ); | ||
|
|
||
| return root; | ||
| } |
There was a problem hiding this comment.
Why is this creating a separate page rather than reusing the site editor (like we do already for patterns for classic themes)
There was a problem hiding this comment.
Ah, that was just a practical workaround for this condition in core, which stops the site editor from loading in a classic theme 😅
The condition wasn't in core yet when we added the patterns page.
I'm more than happy to change this if there's a better solution though!
(Also, long term it might be good to make that condition overridable with a filter or something)
There was a problem hiding this comment.
Why is this creating a separate page rather than reusing the site editor
The assumption here is that it's preferable to use the existing site-editor.php page?
E.g., directly linking from the admin bar to /wp-admin/site-editor.php?path=wp_global_styles
If it cuts down on future maintenance and reduces boilerplate code, it sounds like a good idea.
I'm guessing we'd need to tweak the feature access conditions. E.g., rolling in a wp_theme_has_theme_json check (?)
$is_global_styles_path = isset( $_GET['path'] ) && 'wp_global_styles' === sanitize_key( $_GET['path'] );
if ( ! wp_is_block_theme() ) {
if ( ( ! wp_theme_has_theme_json() && $is_global_styles_path ) || ( ! current_theme_supports( 'block-template-parts' ) && $is_template_part_editor ) ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
} elseif ( ! $is_global_styles_path && ! $is_patterns_editor && ! $is_template_part_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
}
}Loads kinda, but the site editor nav and the layout would probably need some condition boundaries too.
While sniffing about, I noticed there are a couple of related editor settings:
supportsLayout, which is the output ofwp_theme_has_theme_json()anyway, andsupportsTemplatePartsMode, which, as far as I can tell, is not used anywhere in today's Gutenberg or Core.
Also, long term it might be good to make that condition overridable with a filter or something
That makes sense to me - it would make developing from Gutenberg more flexible.
There was a problem hiding this comment.
yes, my main thinking is that I don't want us to create two separate UIs/pages. It's ok to have two different entry points if needed, I just want to share the screens themselves.
There was a problem hiding this comment.
I'm guessing we'd need to tweak the feature access conditions. E.g., rolling in a wp_theme_has_theme_json check (?)
We need more than that, because the stylebook has to be available to classic themes without theme.json too. The end goal is:
- if there's no theme.json, show a static stylebook
- if there's a theme.json with some styles defined, also show global styles controls to edit those styles.
I think it does make sense to not create a separate UI, but I can't work out how to do that given the current core condition prevents the site editor from loading when classic themes are active. If anyone has concrete suggestions for a workaround that doesn't involve a separate UI, they are very welcome!
Otherwise, I'd suggest we go ahead with a temporary separate UI, and update it to use the site editor once core is updated.
There was a problem hiding this comment.
Thanks for investigating and documenting all this, folks.
StyleBook is rendering inside GlobalStylesUIWrapper, which is in the content area. So it's using EditorCanvasContainer to render its contents in the EditorContentSlotFill, which puts it in the preview area.
From memory — I'd have to channel Rob — the style book was added as a slot inside the editor as a matter of convenience. In the editor context, all the block styles are loaded, block editor settings and global styles provider etc.
Consolidate block theme stylebook to not rely on the Editor component
I think that's been a goal for a while, at least to enable swapping it out for the editor, instead jamming it in via the <EditorContentSlotFill.Slot>. It's at least what I was trying to do over in https://github.com/WordPress/gutenberg/pull/66848/files
I'd also hoped that an eventual side-effect of removing this slot would be to remove the editorCanvasView store that determines the slot's contents.
To be honest, I'm not sure how to neatly untangle it either quite yet.
I think for now let's focus on displaying a static stylebook for classic themes without theme.json
👍🏻 Sounds good! We don't have to solve everything right now.
I suppose it'd be neat not to have the change the URL later, say, if we do consolidate and have everything under /wp-admin/site-editor.php?path=/wp_global_styles, but one thing at a time. 😄
It would also be nice if the block theme stylebook view had its own URL, because right now it's impossible to link to it directly.
Something like /wp-admin/site-editor.php?path=/wp_global_styles/style-book?
There was a problem hiding this comment.
In the editor context, all the block styles are loaded, block editor settings and global styles provider etc.
StyleBookPreview is already doing this; essentially the only thing we need for it is BlockEditorProvider with the block editor settings.
The bit I'm least sure about is whether the content preview requires the editor but in theory, being just a preview, it shouldn't. I can look into it once I'm done with this PR. A tentative task list:
- Try splitting out what's currently in
GlobalStylesUIWrapperinto global styles sidebar, content preview and stylebook preview. - Global styles sidebar goes into the content area, the previews in the preview area. Each preview has its own route.
- Content preview is meant to display by default when opening global styles, so it can keep its current
path=%2Fwp_global_stylesURL. - Stylebook preview gains its own URL (
path=/wp_global_styles/style-book?) so we can link to it directly.
I suppose it'd be neat not to have the change the URL later, say, if we do consolidate and have everything under /wp-admin/site-editor.php?path=/wp_global_styles
The obstacle to this is that nothing under site-editor.php can be accessed from a classic theme until we change core. I'm not too fussed about it though; we had the same process with the site editor itself, where it lived in some long winded temp URL before being merged to core and becoming site-editor.php 😄
There was a problem hiding this comment.
The obstacle to this is that nothing under
site-editor.phpcan be accessed from a classic theme until we change core.
Could filtering wp_die() be the solution to this?
I ran into a similar issue when exposing the Patterns page for a classic theme.
This would allow Gutenberg to expose the new site editor paths to classic themes while still maintaining backward compatibility.
There was a problem hiding this comment.
Oh interesting approach, I hadn't thought of that! I'll give it a try.
There was a problem hiding this comment.
Done! This also allowed me to get rid of the separate init function and styles, so now everything is being reused from the site editor itself 🎉
Thanks for the idea @t-hamano!
|
A thought: instead of creating individual menu items, such as Styles and Patterns, that all take you into the immersive view with a mostly empty sidebar, could we simply call the menu item "Design", and include both Styles and Patterns there? 
Mainly on my mind here is #64409 as a followup, which is essentially just one aspect of the Styles section, i.e. "Styles > Typography". CC: @WordPress/gutenberg-design |
tellthemachines
force-pushed
the
try/classic-stylebook
branch
2 times, most recently
from
78b5ca5
to
0d44880
Compare
|
In the mobile layout, when we access the Styles menu, we can't do anything. Is there a way to address this issue?
|
|
@t-hamano I've updated to display the style book only in mobile. |
| [] | ||
| ); | ||
| // Update block editor settings because useMultipleOriginColorsAndGradients fetch colours from there. | ||
| dispatch( blockEditorStore ).updateSettings( siteEditorSettings ); |
There was a problem hiding this comment.
If we wanted to avoid this we could potentially move this logic inside BlockEditorProvider but that would mean adding another layer of component wrapping 😅
@jasmussen do we want to move forward with this in this PR? If so, let's make sure we're on the same page about how it should work:
|
tellthemachines
force-pushed
the
try/classic-stylebook
branch
from
c7f6f9b
to
9f55b77
Compare
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Marking this ready for review as the code is in a good state though there are a couple todo items remaining:
|
tellthemachines
changed the title
|
If we add a new top-level page (Design) for the classic theme, two questions come to my mind:
|
tellthemachines
force-pushed
the
try/classic-stylebook
branch
from
372449a
to
2464969
Compare
| @@ -0,0 +1,44 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Not sure where this file should live; maybe we should put it in 6.8 compat already?
There was a problem hiding this comment.
Good idea.
I guess we'll have to remember to come back and remove the Core action once ported? E.g.,
remove_action( 'admin_init', 'wp_add_styles_submenu_item' );
There was a problem hiding this comment.
It'll have to stay in until we stop supporting any WP version lower than 6.7, but that's what the compat folders are for; we're supposed to audit and remove what's no longer supported after each WP release.
|
Ok peeps nothing like trying it out to see if we like it! I updated the PR to add the Design submenu item under Appearance, which links to the site editor root, and allows access to the Patterns page. Try it out and let me know what you think! classic-stylebook-and-patterns.mp4 |
|
Can't work out why I can also repro the failure in |
|
This seems promising to me.
That's Twenty Eleven. Presumably you get the Styles item if you add a theme.json file? |
What?
Starts addressing #41119.
Adds a "Styles" item to the Appearance menu when a classic theme is enabled, linking to a site editor page showing the stylebook.
Filters
wp_die()to allow the new site editor URL to be accessed by classic themes (temporary, until we can make the change in core)Adds a
StyleBookPreviewcomponent that renders the stylebook as a preview without being wrapped in an editor.The idea for now is to show a static stylebook for classic themes whether or not they have theme.json support. Building on that later I guess we could show the stylebook + global styles if the theme has theme.json, or perhaps make it an optional theme support.
Testing Instructions