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

Jump to bottom

Try: Indicate when text color in post editor is inherited from Global Styles #55952

Closed
wants to merge 5 commits into from
Closed

Try: Indicate when text color in post editor is inherited from Global Styles #55952

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
98c4c0d
Add the experimental setting & first test
michalczaplinski Nov 7, 2023
735ea13
Handle the inherited values correctly
michalczaplinski Nov 7, 2023
e81701f
Merge branch 'trunk' into experiment/show-styles-inherited-values
michalczaplinski Dec 7, 2023
b857070
Merge branch 'trunk' into experiment/show-styles-inherited-values
michalczaplinski Mar 7, 2024
40a4b4b
Remove experimental connections and pattern syncing options
michalczaplinski Mar 7, 2024
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
4 changes: 4 additions & 0 deletions lib/experimental/editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ function gutenberg_enable_experiments() {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalConnections = true', 'before' );
}

if ( $gutenberg_experiments && array_key_exists( 'gutenberg-styles-inheritance-ui', $gutenberg_experiments ) ) {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalStylesInheritanceUI = true', 'before' );
}

if ( gutenberg_is_experiment_enabled( 'gutenberg-no-tinymce' ) ) {
wp_add_inline_script( 'wp-block-library', 'window.__experimentalDisableTinymce = true', 'before' );
}
Expand Down
12 changes: 12 additions & 0 deletions lib/experiments-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,18 @@ function gutenberg_initialize_experiments_settings() {
)
);

add_settings_field(
'gutenberg-styles-inheritance-ui',
__( 'Styles Inheritance UI', 'gutenberg' ),
'gutenberg_display_experiment_field',
'gutenberg-experiments',
'gutenberg_experiments_section',
array(
'label' => __( 'Styles Inheritance UI', 'gutenberg' ),
'id' => 'gutenberg-styles-inheritance-ui',
)
);

register_setting(
'gutenberg-experiments',
'gutenberg-experiments'
Expand Down
32 changes: 28 additions & 4 deletions packages/block-editor/src/components/global-styles/color-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ const popoverProps = {
shift: true,
};

const LabeledColorIndicators = ( { indicators, label } ) => (
const LabeledColorIndicators = ( { indicators, label, hasInheritedValue } ) => (
<HStack justify="flex-start">
<ZStack isLayered={ false } offset={ -8 }>
{ indicators.map( ( indicator, index ) => (
Expand All @@ -164,6 +164,18 @@ const LabeledColorIndicators = ( { indicators, label } ) => (
<FlexItem
className="block-editor-panel-color-gradient-settings__color-name"
title={ label }
// If it has an inherited value, we want to label with purple background
// and purple text.
style={
hasInheritedValue
? {
backgroundColor: '#FAF5FE',
color: '#7A00DF',
borderRadius: 3,
padding: '4px 3px',
}
: undefined
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting, just noting that the "inheritValue" prop is a common prop to all the *Panel components and that in global styles for "block panels" it's possible to be set with the value set at the "root level" of global styles for instance.

So basically, the behavior you're introducing here is also going to trigger in the global styles sidebar for blocks. I think that's probably fine but it's something to be aware of.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the behavior you're introducing here is also going to trigger in the global styles sidebar for blocks

yup, thanks for pointing that out! I noticed it as well, but I haven't tried to fix it at all yet.

>
{ label }
</FlexItem>
Expand All @@ -177,14 +189,15 @@ function ColorPanelTab( {
setValue,
colorGradientControlSettings,
} ) {
const value = userValue || inheritedValue;
return (
<ColorGradientControl
{ ...colorGradientControlSettings }
showTitle={ false }
enableAlpha
__experimentalIsRenderedInSidebar
colorValue={ isGradient ? undefined : inheritedValue }
gradientValue={ isGradient ? inheritedValue : undefined }
colorValue={ isGradient ? undefined : value }
gradientValue={ isGradient ? value : undefined }
onColorChange={ isGradient ? undefined : setValue }
onGradientChange={ isGradient ? setValue : undefined }
clearable={ inheritedValue === userValue }
Expand All @@ -196,6 +209,7 @@ function ColorPanelTab( {
function ColorPanelDropdown( {
label,
hasValue,
hasInheritedValue,
resetValue,
isShownByDefault,
indicators,
Expand Down Expand Up @@ -242,6 +256,7 @@ function ColorPanelDropdown( {
<LabeledColorIndicators
indicators={ indicators }
label={ label }
hasInheritedValue={ hasInheritedValue }
/>
</Button>
);
Expand Down Expand Up @@ -512,8 +527,9 @@ export default function ColorPanel( {
label: __( 'Text' ),
hasValue: hasTextColor,
resetValue: resetTextColor,
hasInheritedValue: !! textColor && ! userTextColor,
isShownByDefault: defaultControls.text,
indicators: [ textColor ],
indicators: [ userTextColor || textColor ],
tabs: [
{
key: 'text',
Expand All @@ -528,6 +544,10 @@ export default function ColorPanel( {
key: 'background',
label: __( 'Background' ),
hasValue: hasBackground,
hasInheritedValue:
( !! backgroundColor || !! gradient ) &&
! userBackgroundColor &&
! userGradient,
resetValue: resetBackground,
isShownByDefault: defaultControls.background,
indicators: [ gradient ?? backgroundColor ],
Expand All @@ -553,6 +573,10 @@ export default function ColorPanel( {
key: 'link',
label: __( 'Link' ),
hasValue: hasLink,
hasInheritedValue:
( !! linkColor || !! hoverLinkColor ) &&
! userLinkColor &&
! userHoverLinkColor,
resetValue: resetLink,
isShownByDefault: defaultControls.link,
indicators: [ linkColor, hoverLinkColor ],
Expand Down
5 changes: 5 additions & 0 deletions packages/block-editor/src/hooks/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import {
default as StylesColorPanel,
} from '../components/global-styles/color-panel';
import BlockColorContrastChecker from './contrast-checker';
import { useGlobalStyle } from '../components/global-styles';

export const COLOR_SUPPORT_KEY = 'color';

Expand Down Expand Up @@ -293,6 +294,9 @@ export function ColorEdit( props ) {
const { clientId, name, attributes, setAttributes } = props;
const settings = useBlockSettings( name );
const isEnabled = useHasColorPanel( settings );

const [ userValue ] = useGlobalStyle( '', props.name, 'user' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I do think we need some kind of useGlobalStyle hook in the block editor or maybe just useStyle since we already have the useSetting one for the "settings".

So yeah I wouldn't use the "global styles provider" and useGlobalStyle or useGlobalSettings as they exist today. In fact these two hooks were in edit-site as well, they have been moved to block-editor but as you can check they're only used in edit-site, so while I believe the goal was to "merge them" with "useSetting" and a potential "useStyle" that is block editor specific, this was never done. So at the moment, I consider "useGlobalStyle" and "useSetting" as edit-site specific things just like the provider. (I think it was @noisysocks that moved them, I forgot the entire reasoning)

I guess in my mind, we need to introduce a new key to settings prop of BlockEditorProvider that should contain the global styles "styles" property (we only have settings right now) and use that property to build a block-editor version of useGlobalStyle hook?

Maybe I'm missing something here so would love to hear more thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to introduce a new key to settings prop of BlockEditorProvider that should contain the global styles "styles" property (we only have settings right now) and use that property to build a block-editor version of useGlobalStyle hook?

Yes! This would certainly fulfill the needs of the current PR (and the larger styles inheritance UI system). I don't know enough to know if this potentially introduces any problems though.

Btw, is there any overlap here with #55970 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@michalczaplinski not necessarily, the problem here is a bit different than sharing the same code between edit-post and edit-site, we want access to the style hierarchy in the "block-editor" package. Until now, the block editor was only aware of the editor styles as CSS (styles property in the settings prop) but now we want the theme.json structure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dug here a little bit and these are the ideas I tried:

1. Create a useStyles() hook in the block-editor to get the styles and pass them to the BlockEditorProvider.

The GlobalStylesProvider that I used in the current PR relies on useGlobalStylesUserConfig():

gutenberg/packages/edit-site/src/components/global-styles/global-styles-provider.js

Lines 33 to 34 in 40a4b4b

function useGlobalStylesUserConfig() {
const { globalStylesId, isReady, settings, styles } = useSelect(

which in turn loads the global styles from the core-data store:

gutenberg/packages/edit-site/src/components/global-styles/global-styles-provider.js

Lines 40 to 46 in 40a4b4b

const record = _globalStylesId
? getEditedEntityRecord(
'root',
'globalStyles',
_globalStylesId
)
: undefined;

However, we cannot use the core-data package in the block-editor either...

2. Pass the data to the settings on the server

My second thought was to see if we can pass the required styles on the server in PHP in the block-editor-settings.php. However, the Global Styles are loaded via the REST API so that's not an option either as far as I can tell.

3. Create a useStyles() hook in the editor package and use it in the edit-post to inject the styles.

This approach looks the most reasonable to me as we can use core-data in the editor package. I opened a new PR in #59929 following this approach.

However, I'm still not sure if that's the best way to solve the problem:

  1. Having distinct logic for getting the styles in the edit-site and edit-post does not seem ideal.
  2. It seems a bit out of place to have styles-specific logic in edit-post/src/editor.js

I'd appreciate your guidance here @youknowriad! 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

For me, the best approach is probably an additional "setting" to the settings prop of the BlockEditorProvider.

  • We have __experimentalFeatures (which we should probably rename at some point for the "settings" part of theme.json
  • We need the same but for "styles".

The problem is going to be the naming of these settings and I'm not the best person when it comes to naming things tbh 😬. cc @mcsf maybe

Copy link
Contributor

Choose a reason for hiding this comment

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

@michalczaplinski you're right, sorry I missed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I appreciate the guidance! I'm glad that we had roughly the same idea 🙂 We can continue to discuss in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

it is also possible to combine both "__experimentalFeatures" and styles in a single prop we could name theme maybe but I'm not sure entirely. I'd defer to others.

How about template? :trollface:

More seriously, I think this should be an opportunity to inventory everything that goes into __experimentalFeatures (as someone who isn't touching these pieces all the time, it's hard to remember what goes where). I think I'd favour a solution that strives for consistency with theme.json, since theme.json is already established as the dominant mental model that people need to get comfortable with, so we might as well piggyback off that.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Since the question was about naming, this was implicit but omitted in my comment: it would be nice to be able to express things in terms of styles & settings)

Copy link
Contributor Author

@michalczaplinski michalczaplinski Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How about template? :trollface:

I'll do you one better: Template hooks 😄

Makes sense @mcsf. I've closed this PR in favour of #59929 which is the approach @youknowriad suggested. Let's continue to discuss there 🙂 .


const value = useMemo( () => {
return attributesToStyle( {
style: attributes.style,
Expand Down Expand Up @@ -337,6 +341,7 @@ export function ColorEdit( props ) {
<StylesColorPanel
as={ ColorInspectorControl }
panelId={ clientId }
inheritedValue={ userValue }
settings={ settings }
value={ value }
onChange={ onChange }
Expand Down
33 changes: 18 additions & 15 deletions packages/edit-post/src/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import { SlotFillProvider } from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { store as preferencesStore } from '@wordpress/preferences';
import { CommandMenu } from '@wordpress/commands';
import { GlobalStylesProvider } from '@wordpress/edit-site';

/**
* Internal dependencies
Expand Down Expand Up @@ -151,21 +152,23 @@ function Editor( { postId, postType, settings, initialEdits, ...props } ) {

return (
<SlotFillProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={ isTemplateMode ? template : undefined }
{ ...props }
>
<ErrorBoundary>
<CommandMenu />
<EditorInitialization postId={ postId } />
<Layout />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
<GlobalStylesProvider>
<ExperimentalEditorProvider
settings={ editorSettings }
post={ post }
initialEdits={ initialEdits }
useSubRegistry={ false }
__unstableTemplate={ isTemplateMode ? template : undefined }
{ ...props }
>
<ErrorBoundary>
<CommandMenu />
<EditorInitialization postId={ postId } />
<Layout />
</ErrorBoundary>
<PostLockedModal />
</ExperimentalEditorProvider>
</GlobalStylesProvider>
</SlotFillProvider>
);
}
Expand Down
1 change: 1 addition & 0 deletions packages/edit-site/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,4 @@ export { default as PluginSidebarMoreMenuItem } from './components/header-edit-m
export { default as PluginMoreMenuItem } from './components/header-edit-mode/plugin-more-menu-item';
export { default as PluginTemplateSettingPanel } from './components/plugin-template-setting-panel';
export { store } from './store';
export { GlobalStylesProvider } from './components/global-styles/global-styles-provider';
Loading