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.

Sign up for GitHub

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

Global Styles: synchronize user CPT registration and UI visibility #35427

Merged
merged 5 commits into from
Oct 7, 2021
Merged

Global Styles: synchronize user CPT registration and UI visibility #35427

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4850fab
Pass the setting down
oandregal Oct 7, 2021
8404a6d
Use the flag in the client to hide the GS UI
oandregal Oct 7, 2021
9e5aeb5
Fix PHPDoc
oandregal Oct 7, 2021
7d14040
Tie GS to the site editor being available
oandregal Oct 7, 2021
239da35
Revert "Use the flag in the client to hide the GS UI"
oandregal Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions lib/global-styles.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,7 @@ function gutenberg_experimental_global_styles_settings( $settings ) {
is_callable( 'get_current_screen' ) &&
function_exists( 'gutenberg_is_edit_site_page' ) &&
gutenberg_is_edit_site_page( get_current_screen()->id ) &&
WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() &&
gutenberg_supports_block_templates()
gutenberg_experimental_is_global_styles_available_to_user()
) {
$context = 'site-editor';
}
Expand Down Expand Up @@ -171,6 +170,8 @@ function_exists( 'gutenberg_is_edit_site_page' ) &&
$settings['styles'] = $styles_without_existing_global_styles;
}

$settings['isGlobalStylesUIEnabled'] = gutenberg_experimental_is_global_styles_available_to_user();

// Copied from get_block_editor_settings() at wordpress-develop/block-editor.php.
$settings['__experimentalFeatures'] = $consolidated->get_settings();

Expand Down Expand Up @@ -231,17 +232,25 @@ function_exists( 'gutenberg_is_edit_site_page' ) &&
return $settings;
}

/**
* Whether or not the Global Styles sidebar is available to the user
* and the user CPT is registered.
*
* @return boolean
*/
function gutenberg_experimental_is_global_styles_available_to_user() {
return WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() || gutenberg_supports_block_templates();
Copy link
Member Author

@oandregal oandregal Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@oandregal oandregal Oct 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I pushed changes to make global styles (UI+CPT) use the same check than the site editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just noticed that you removed the frontend checks nevermind

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. To confirm: both site editor & GS are now shown under the same conditions with this PR (gutenberg_is_fse_theme).

}

/**
* Register CPT to store/access user data.
*
* @return array|undefined
*/
function gutenberg_experimental_global_styles_register_user_cpt() {
if ( ! WP_Theme_JSON_Resolver_Gutenberg::theme_has_support() ) {
return;
if ( gutenberg_experimental_is_global_styles_available_to_user() ) {
WP_Theme_JSON_Resolver_Gutenberg::register_user_custom_post_type();
}

WP_Theme_JSON_Resolver_Gutenberg::register_user_custom_post_type();
}

/**
Expand Down
6 changes: 6 additions & 0 deletions packages/edit-site/src/components/global-styles/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,3 +345,9 @@ export function getSupportedGlobalStylesPanels( name ) {

return supportKeys;
}

export function useGlobalStylesIsEnabled() {
return useSelect( ( select ) => {
return select( editSiteStore ).getSettings().isGlobalStylesUIEnabled;
}, [] );
}
7 changes: 6 additions & 1 deletion packages/edit-site/src/components/global-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
export { default as GlobalStylesUI } from './ui';
export { useGlobalStylesReset, useStyle, useSetting } from './hooks';
export {
useGlobalStylesIsEnabled,
useGlobalStylesReset,
useStyle,
useSetting,
} from './hooks';
export { useGlobalStylesOutput } from './use-global-styles-output';
9 changes: 8 additions & 1 deletion packages/edit-site/src/components/sidebar/global-styles-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,17 @@ import { styles } from '@wordpress/icons';
* Internal dependencies
*/
import DefaultSidebar from './default-sidebar';
import { GlobalStylesUI, useGlobalStylesReset } from '../global-styles';
import {
useGlobalStylesIsEnabled,
GlobalStylesUI,
useGlobalStylesReset,
} from '../global-styles';

export default function GlobalStylesSidebar() {
const [ canRestart, onReset ] = useGlobalStylesReset();
if ( ! useGlobalStylesIsEnabled() ) {
return null;
}

return (
<DefaultSidebar
Expand Down