Update: Use wp_theme taxonomy for wp_global_styles cpt. #31436
Merged
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
jorgefilipecosta
added
the
Global Styles
jorgefilipecosta
requested review from
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: 0 B Total Size: 1.31 MB ℹ️ View Unchanged
|
oandregal
reviewed
May 4, 2021
| @@ -90,7 +90,7 @@ function gutenberg_register_wp_theme_taxonomy() { | |||
|
|
|||
| register_taxonomy( | |||
| 'wp_theme', | |||
| array( 'wp_template', 'wp_template_part' ), | |||
| array( 'wp_template', 'wp_template_part', 'wp_global_styles' ), | |||
There was a problem hiding this comment.
There may be themes that have theme.json but aren't block-based themes and vice-versa, so we need to make sure that this relationship works as expected in those scenarios (a few lines above this there's a gutenberg_supports_block_templates check that makes this only work on block-based themes).
There was a problem hiding this comment.
Good catch I updated the condition used to register the taxonomy.
There was a problem hiding this comment.
Is it ok to register all of this or should we only register the individual pieces per each feature?
- register
wp_global_stylesif a theme hastheme.jsonsupport - register
wp_templateandwp_template_partif a theme has block-based templates
cc @vindl
There was a problem hiding this comment.
Here we are not registering wp_global_styles or wp_template we are registering wp_theme taxonomy and saying the taxonomy can be used on wp_global_styles, wp_template and wp_template_part block types. I think there is no issue in registering a taxonomy and saying it can be used in a CPT that is not yet available.
There was a problem hiding this comment.
At #35427 we're probably making changes to when we register the user CPT. I've checked the logic for gutenberg_register_wp_theme_taxonomy and it still works as expected. Just wanted to make a note in case I've missed anything.
jorgefilipecosta
force-pushed
the
update/use-wp_theme-taxonomy-for-wp_global_styles-cpt
branch
from
ae5034e to
3368200
Compare
oandregal
force-pushed
the
update/use-wp_theme-taxonomy-for-wp_global_styles-cpt
branch
from
3368200 to
3d28b60
Compare
|
Hey, was testing this and observed some weird things. Then I've noticed this wasn't built upon the latest changes (site-editor + theme.json format) so went ahead and rebased this branch to make sure I can test this PR in isolation properly. Hope the rebase didn't mess up your local setup :) |
oandregal
reviewed
May 7, 2021
| @@ -311,15 +311,20 @@ public static function get_theme_data( $theme_support_data = array() ) { | |||
| private static function get_user_data_from_custom_post_type( $should_create_cpt = false, $post_status_filter = array( 'publish' ) ) { | |||
| $user_cpt = array(); | |||
| $post_type_filter = 'wp_global_styles'; | |||
| $post_name_filter = 'wp-global-styles-' . urlencode( wp_get_theme()->get_stylesheet() ); | |||
There was a problem hiding this comment.
Braindump: the result of removing this is that the CPT name will inherit the title's, which is "Custom Styles", so we end up with CPTs with names (and guids) such as custom-styles, custom-styles-2, custom-styles-3, etc. Wondering if we could retain this or even have custom-styles-<theme> as a name.
Not that it matters much for our use as we'll have the taxonomy. However, I wonder what happens if/when we reach the last guid number. Does it start over from 0? At that point, is it ok that there are two CPTs with custom-styles-2 names? Those sort of situations.
There was a problem hiding this comment.
Yes, I guess we can keep the existing name, I updated the code.
oandregal
approved these changes
May 7, 2021
jorgefilipecosta
force-pushed
the
update/use-wp_theme-taxonomy-for-wp_global_styles-cpt
branch
from
3d28b60 to
9dade09
Compare
aristath
approved these changes
May 10, 2021
jorgefilipecosta
deleted the
update/use-wp_theme-taxonomy-for-wp_global_styles-cpt
branch
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.
Templates and template parts rely on wp_theme taxonomy to associate template and template posts with a given theme. Global styles was using a different mechanism (appending theme id to the name of the post). This PR makes global styles use the same mechanism being used on templates and template parts.
Fixes:#30191
How has this been tested?
I enabled a global styles theme.
I went to the site editor. I applied some user changes to the global styles and saved them.
I verified the changes were applied to the front end.
I reload the site editor and verified the changes were still present.
I switched to another global style enable theme.
I verified my previous changes did not appeared anymore-
I applied other changes and verified they were applied on the front and persisted after site editor reload.
I switched back to the initial theme and loaded the site editor.
I verified the changes I made while that theme was enabled appeared again.