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.

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

Add settings hook #40547

Merged
merged 6 commits into from
Apr 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions packages/block-editor/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,10 @@ _Parameters_

### useSetting

Hook that retrieves the editor setting.
It works with nested objects using by finding the value at path.
Hook that retrieves the given setting for the block instance in use.

It looks up the settings first in the block instance hierarchy.
If none is found, it'll look it up in the block editor store.

_Usage_

Expand Down
78 changes: 57 additions & 21 deletions packages/block-editor/src/components/use-setting/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import { get } from 'lodash';
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { __EXPERIMENTAL_PATHS_WITH_MERGE as PATHS_WITH_MERGE } from '@wordpress/blocks';
import {
__EXPERIMENTAL_PATHS_WITH_MERGE as PATHS_WITH_MERGE,
hasBlockSupport,
} from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -91,8 +94,10 @@ const removeCustomPrefixes = ( path ) => {
};

/**
* Hook that retrieves the editor setting.
* It works with nested objects using by finding the value at path.
* Hook that retrieves the given setting for the block instance in use.
*
* It looks up the settings first in the block instance hierarchy.
* If none is found, it'll look it up in the block editor store.
*
* @param {string} path The path to the setting.
* @return {any} Returns the value defined for the setting.
Expand All @@ -102,7 +107,7 @@ const removeCustomPrefixes = ( path ) => {
* ```
*/
export default function useSetting( path ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can unit test this hook to avoid regressions. Wrap it in a test component and mock the global styles input and see what you get in different situations.

const { name: blockName } = useBlockEditContext();
const { name: blockName, clientId } = useBlockEditContext();

const setting = useSelect(
( select ) => {
Expand All @@ -113,42 +118,73 @@ export default function useSetting( path ) {
);
return undefined;
}
const settings = select( blockEditorStore ).getSettings();

// 1 - Use __experimental features, if available.
// We cascade to the all value if the block one is not available.
let result;
const normalizedPath = removeCustomPrefixes( path );
const defaultsPath = `__experimentalFeatures.${ normalizedPath }`;
const blockPath = `__experimentalFeatures.blocks.${ blockName }.${ normalizedPath }`;
const experimentalFeaturesResult =
get( settings, blockPath ) ?? get( settings, defaultsPath );

if ( experimentalFeaturesResult !== undefined ) {
// 1. Take settings from the block instance or its ancestors.
Copy link
Member Author

Choose a reason for hiding this comment

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

The algorith for useSetting is:

  • lookup for settings defined in the block instance or any of its ancestors => this is the only behavioral change introduced by this PR.
  • lookup for settings defined in the block editor store
  • fallback for deprecated settings
  • fallback for dropcap

Copy link
Contributor

Choose a reason for hiding this comment

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

In global styles, I added a custom palette to the "paragraph" block and then selected a paragraph block from your example above. I wasn't sure what should show up:

  • The palette of the parent "group"
  • The palette of the "paragraph" block from Global styles.

I think that's the remaining point we should figure out in the algorithm, we should probably just pick one approach and stick with it.

My first thought is that picking the "paragraph" settings from the global styles might make more sense here. (Right now it picks the parent's palette)

cc @jorgefilipecosta @mcsf

Copy link
Member Author

@oandregal oandregal Apr 26, 2022

Choose a reason for hiding this comment

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

I shared some longer thoughts for this at #40318 (comment)

I've looked at use cases for sections, aka why do we want to do this? I understand we want to enable doing things such as "set colors for this whole section" or "disable custom font sizes for the whole section". These are easier to do with a "by source" algorithm (first considers block instance and its ancestors, then theme.json data). Specially when/if we expose this to the users via an UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not that simple. The current algorithm is different that "style" and it's not clear why we'd want "settings" to work differently.

If you apply a text color to a paragraph block, it won't be using the color from the parent "style", it will continue to use the "global styles" one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but styles and settings behave different in theme.json as well: styles aren't inherited and settings are .

Copy link
Member Author

@oandregal oandregal Apr 28, 2022

Choose a reason for hiding this comment

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

The CSS model allows us to choose between still keeping the table palette or overwriting anyway. On the alternative model, I don't really have a choice.

You do! You just set the settings for the specific block types you're interested in (and the other ones -the table- will fall back to the store settings).

I think the crux of the conversation is: what are the common uses case we should optimize for? If this one is the typical thing patterns want to do and the table example you shared is the uncommon one, the model this PR follows works best.

what is the solution we are going to have to on the example I gave

I've created a pen that demonstrates the challenge you shared and how we can address it https://codepen.io/oandregal/pen/eYyqxZO?editors=1100

Let me unwrap it here as well. Given a section like this one (note the paragraph has a red color applied by the user):

  <div class="wp-section-UUID">
    <p class="has-red-color"></p>
  </div>

If the theme has added some paragraph-specific settings, as in:

{
  "settings": {
    "color": {
      "palette": [
	    { "slug": "red", "color": "red-theme-global" }
      ]
    },
    "blocks": {
      "core/paragraph": {
        "color": {
          "palette": [
            { "slug": "red", "color": "red-theme-block" }
          ]
        }
      }
    }
  }
}

And the section has this:

{
  "settings": {
    "color": {
      "palette": [
        { "slug": "red", "color": "red-section-global" }
      ]
    }
  }
}

How do we make it so the color rendered for the paragraph is red-section-global?

/*
 * THIS IS THE CSS GENERATED FOR THE `theme.json`
 */
body {
  --wp--preset--color--red: "red-theme-global";
}
p {
  --wp--preset--color--red: "red-theme-block";
}
.has-red-color {
  color: var(--wp--preset--color--red) !important;
}

/*
 * THIS IS THE CSS GENERATED FOR THE SECTION.
 */
.wp-section-UUID,
.wp-section-UUID [class*="wp-block"],
/*
 * We also need fallbacks for any block that don't use the .wp-block-* single selector
 * but instead use __experimentalSelector to define their own. These include
 * paragraph, list (li, ul), heading (h1-h6), and table so far.
 */
.wp-section-UUID p,
.wp-section-UUID li,
.wp-section-UUID ul,
.wp-section-UUID h1,
.wp-section-UUID h2,
.wp-section-UUID h3,
.wp-section-UUID h4,
.wp-section-UUID h5,
.wp-section-UUID h6,
.wp-section-UUID .wp-block-table > table {
  --wp--preset--color--red: "red-section-global";
}

Does this address your concerns?

Note that this PR doesn't generate the styles for the presets declared in a section. That's something that has a complexity of its own and I thought that to be a follow-up (things like generating the preset classes or not depending on whether the preset is already defined in theme.json, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ongoing draft PR for rendering the settings at #40683

Copy link
Member

Choose a reason for hiding this comment

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

You do! You just set the settings for the specific block types you're interested in (and the other ones -the table- will fall back to the store settings).

Your section may not be aware of the blocks it is interested in, it may have a group where the user can nest anything inside.

I think the crux of the conversation is: what are the common uses case we should optimize for? If #40547 (comment) is the typical thing patterns want to do and the table example you shared is the uncommon one, the model this PR follows works best.

I agree we should optimize for the most common cause. The thing is we don't know the most common case yet. I think a theme setting block specific settings is something rare and very specific that should be respected even if that block is used inside a section setting something global for that setting. Your expectation is that something global at the section should take priority above a theme-specific setting. I guess we need to choose and iterated and if we receive feedback or new information later change the direction if that's a possibility.

How do we make it so the color rendered for the paragraph is red-section-global?

.wp-section-UUID,
.wp-section-UUID [class*="wp-block"], /* Any child block */
/*
 * We also need to cover for child blocks that don't use the .wp-block-* class:
 * paragraph, list (ul, or), and heading (h1-h6).
 */
.wp-section-UUID p,
.wp-section-UUID li,
.wp-section-UUID ul,
.wp-section-UUID h1,
.wp-section-UUID h2,
.wp-section-UUID h3,
.wp-section-UUID h4,
.wp-section-UUID h5,
.wp-section-UUID h6,

The solution proposed makes things work but requires us to ship much more CSS code, for every color or font size, etc instead of just need to use just .wp-section-UUID, we need to use a much bigger selector per preset we have that means more bytes sent over the wire for every section that has some preset.

I'm not sure of the best approach either so I'm fine with giving a try to the solution we have here and seeing how it works in practice.

Copy link
Member Author

@oandregal oandregal Apr 28, 2022

Choose a reason for hiding this comment

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

Actually, there's a simpler CSS for the presets generated by the section if we embrace how theme.json works:

/*
 * CSS Custom Properties defined in the section
 */
.wp-section-UUID .has-red-color {
  --wp--preset--color--red: "red-section-global";
}

/*
 * CSS classes for presets defined in the section.
 * Ideally, we should not enqueue the classes that are already enqueued by theme.json.
 * Though we still need to check at runtime because a given theme may have them
 * but a different theme may not.
*/
.wp-section-UUID .has-red-color { color: var(--wp--preset--color--red) !important; } 

That's it (codepen).

I think we can decide which of the alternatives we want to pursue in the PR that implements this.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Apr 28, 2022

Choose a reason for hiding this comment

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

Yes but if have a block specific color, on theme.json we generate:

p.has-red-color {
color: var(--wp--preset--color--paragraph-red)
}

So in each section we can not do just:

.wp-section-UUID .has-red-color {
  --wp--preset--color--red: "red-section-global";
}

We also need to overwrite the block specific colors:

.wp-section-UUID p.has-red-color {
  --wp--preset--color--red: "red-section-global";
}

const candidates = [
...select( blockEditorStore ).getBlockParents( clientId ),
clientId, // The current block is added last, so it overwrites any ancestor.
];
candidates.forEach( ( candidateClientId ) => {
const candidateBlockName = select(
blockEditorStore
).getBlockName( candidateClientId );
if (
hasBlockSupport(
candidateBlockName,
'__experimentalSettings',
false
)
) {
const candidateAtts = select(
blockEditorStore
).getBlockAttributes( candidateClientId );
const candidateResult =
get(
candidateAtts,
`settings.blocks.${ blockName }.${ normalizedPath }`
) ??
get( candidateAtts, `settings.${ normalizedPath }` );
if ( candidateResult !== undefined ) {
result = candidateResult;
}
}
} );

// 2. Fall back to the settings from the block editor store (__experimentalFeatures).
const settings = select( blockEditorStore ).getSettings();
if ( result === undefined ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The only change in this section is to rename __experimentalFeaturesResult to result.

const defaultsPath = `__experimentalFeatures.${ normalizedPath }`;
const blockPath = `__experimentalFeatures.blocks.${ blockName }.${ normalizedPath }`;
result =
get( settings, blockPath ) ?? get( settings, defaultsPath );
}

// Return if the setting was found in either the block instance or the store.
if ( result !== undefined ) {
if ( PATHS_WITH_MERGE[ normalizedPath ] ) {
return (
experimentalFeaturesResult.custom ??
experimentalFeaturesResult.theme ??
experimentalFeaturesResult.default
);
return result.custom ?? result.theme ?? result.default;
}
return experimentalFeaturesResult;
return result;
}

// 2 - Use deprecated settings, otherwise.
// 3. Otherwise, use deprecated settings.
const deprecatedSettingsValue = deprecatedFlags[ normalizedPath ]
? deprecatedFlags[ normalizedPath ]( settings )
: undefined;
if ( deprecatedSettingsValue !== undefined ) {
return deprecatedSettingsValue;
}

// 3 - Fall back for typography.dropCap:
// 4. Fallback for typography.dropCap:
// This is only necessary to support typography.dropCap.
// when __experimentalFeatures are not present (core without plugin).
// To remove when __experimentalFeatures are ported to core.
return normalizedPath === 'typography.dropCap' ? true : undefined;
},
[ blockName, path ]
[ blockName, clientId, path ]
);

return setting;
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import './anchor';
import './custom-class-name';
import './generated-class-name';
import './style';
import './settings';
import './color';
import './duotone';
import './font-size';
Expand Down
32 changes: 32 additions & 0 deletions packages/block-editor/src/hooks/settings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import { hasBlockSupport } from '@wordpress/blocks';

const hasSettingsSupport = ( blockType ) =>
hasBlockSupport( blockType, '__experimentalSettings', false );

function addAttribute( settings ) {
if ( ! hasSettingsSupport( settings ) ) {
return settings;
}

// Allow blocks to specify their own attribute definition with default values if needed.
if ( ! settings?.attributes?.settings ) {
settings.attributes = {
...settings.attributes,
settings: {
type: 'object',
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This works :) I wonder if this kind of built-in attributes we're adding (style, lock, settings) should at some point move to the registration function instead of using hooks. They're becoming so central to the block editor experience. (A discussion for later)

Copy link
Member

Choose a reason for hiding this comment

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

It's already a thing in WordPress core for the lock attribute:

https://github.com/WordPress/wordpress-develop/blob/04e9728701b3dde0260c7cdebfd726edbe62ac97/src/wp-includes/class-wp-block-type.php#L213-L215

I fully agree we should follow with style and className which are essential for styling all blocks.

Copy link
Member

Choose a reason for hiding this comment

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

It's worth noting that a couple of blocks can't use style and className, as they lack the markup to apply them to, e.g. the Page Break and Shortcode blocks. Not sure if that matters here, but I figured I'd bring it up.

}

return settings;
}

addFilter(
'blocks.registerBlockType',
'core/settings/addAttribute',
addAttribute
);
48 changes: 48 additions & 0 deletions packages/block-editor/src/hooks/test/settings.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/**
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import '../settings';

describe( 'with settings', () => {
const blockSettings = {
save: () => <div className="default" />,
category: 'text',
title: 'block title',
};

describe( 'addAttribute', () => {
const addAttribute = applyFilters.bind(
null,
'blocks.registerBlockType'
);

it( 'does not have settings att if settings block support is not enabled', () => {
const settings = addAttribute( {
...blockSettings,
supports: {
__experimentalSettings: false,
},
} );

expect( settings.attributes ).toBe( undefined );
} );

it( 'has settings att if settings block supports is enabled', () => {
const settings = addAttribute( {
...blockSettings,
supports: {
__experimentalSettings: true,
},
} );

expect( settings.attributes ).toStrictEqual( {
settings: { type: 'object' },
} );
} );
} );
} );
1 change: 1 addition & 0 deletions packages/block-library/src/group/block.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
}
},
"supports": {
"__experimentalSettings": true,
"align": [ "wide", "full" ],
"anchor": true,
"html": false,
Expand Down