Global Styles: synchronize user CPT registration and UI visibility #35427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
commented
Oct 7, 2021
lib/global-styles.php
Outdated
| * @return boolean | ||
| */ | ||
| function gutenberg_experimental_is_global_styles_available_to_user() { | ||
| return WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() || gutenberg_supports_block_templates(); |
There was a problem hiding this comment.
This is where we decide under which conditions the UI and the user CPT are available.
At #34885 (comment) I argued that I lean towards only enable them when the user has a theme.json, otherwise, we don't know whether the user styles are going to play well with the theme's. Riad brought up we should open this up for themes that have block templates but don't have theme.json as well.
As long as both places use the same logic, I'm not strongly opinionated. I went here with the current logic of the UI.
If we go with theme.json or block templates, I do wonder where's the logic we use to decide whether the edit-site is available. Shouldn't we tie this into that?
There was a problem hiding this comment.
I've just checked and we don't load the site editor if the theme does not support block templates. So, if we want to update the logic, block templates are the only thing that matters to this as well.
There was a problem hiding this comment.
Should it be be gutenberg_supports_block_templates or gutenberg_is_fse_theme. Meaning should classic themes that support block templates allowed to have global styles? Leaning towards no but curious what you think.
There was a problem hiding this comment.
We enable the site editor only for themes that actually have templates (gutenberg_is_fse_theme), so this should be enough of a check to enable GS as well. As a curiosity, support for block-templates is added by us if the theme has a theme.json, so it's a redundant if we already checked for theme.json support.
However, note that this starts to get convoluted when/if we want embed GS in other screens (template or post editor). It's a lot simpler (and safer in terms of styling, in my view) if we only enable GS for themes with theme.json.
There was a problem hiding this comment.
Ok, I pushed changes to make global styles (UI+CPT) use the same check than the site editor.
There was a problem hiding this comment.
Ok so as it stands, this PR means Global Styles UI is only for FSE themes right? Meaning a classic theme with theme.json won't have access to it (if we potentially move the global styles UI elsewhere, say to the post editor or the customizer). That's fine by me for now but it also make me wonder why we need the check in the frontend since basically the site editor is not rendered in the exact same conditions.
There was a problem hiding this comment.
Oh I just noticed that you removed the frontend checks nevermind
There was a problem hiding this comment.
Right. To confirm: both site editor & GS are now shown under the same conditions with this PR (gutenberg_is_fse_theme).
|
Size Change: +33 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
oandregal
added
[Type] Bug
oandregal
commented
Oct 7, 2021
| @@ -94,7 +94,7 @@ function gutenberg_menu() { | |||
| * @since 9.4.0 | |||
| */ | |||
| function gutenberg_site_editor_menu() { | |||
| if ( gutenberg_is_fse_theme() ) { | |||
| if ( gutenberg_experimental_is_site_editor_available() ) { | |||
There was a problem hiding this comment.
I could have avoided this change by using gutenberg_is_fse_theme directly is GS code. However, it feels like if the edit site & GS are connected "conceptually" as we're arguing, they should also be explicitly connected by code. So, when/if our future selves want to update this logic is clear what it affects to.
oandregal
changed the title
youknowriad
approved these changes
Oct 7, 2021
|
I'm milestoning this for 11.7, given that it fixes a bug that started happening in 11.6. |
oandregal
added a commit
that referenced
this pull request
Oct 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow-up to #34885 (comment)
This PR centralizes the logic to decide whether to register the user CPT for global styles and whether to make the Global Styles UI visible. It also updates when the user CPT is registered: when the theme has a
theme.jsonor block templates, following recent changes in the sidebar.Context
The Global Styles sidebar is the tool the user interacts with to provide their own global styles. These styles are stored in a CPT. The registration of the user CPT and the visibility of the GS styles sidebar should go in sync: if the user CPT is not registered the sidebar should not be visible either. We had separate checks in the server and client for this. When we made the GS UI available for all themes that could load the site editor at #34885 the user CPT was still only registered for themes with
theme.json, hence we had failures in some themes because the CPT was not registered.How to test
Test that it works for a theme with
theme.jsonand block templates:Test that it works for a theme with block templates but not
theme.json:theme.jsonfrom the empty theme and reload the site editor.Test that it works for a theme with
theme.jsonbut not block templates:theme.jsonof the theme.block-templates/index.htmland reload the site editor.