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

Block Editor: Allow control over drop cap feature with useEditorFeature helper #22291

Merged
merged 2 commits into from
May 27, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 12, 2020

Description

Related to #21583.

So far there is one new field proposed in the editor (and block editor) settings on the client: __experimentalFeatures. It's exposed from the server.

Usage:

// edit.js

const Edit = ( props ) => {
    const isDisabled = ! useEditorFeature( 'typography.dropCap' );
    // ...
};

The discussion about the shape of config files needs to happen first before we commit to the more solid proposal.
There was a very in-depth discussion about the shape of the theme config (and the WordPress core config with defaults for themes) in this thread: #22291 (review). We didn't commit to the final shape but we have enough to continue work on this feature and similar concepts related to themes and way to have more control over the block editor.

Finally, I tried to move supports from the JS to block.json for all blocks in #22422. At the same time, I wanted to try to rename it to features but it ended up as a very deep refactoring so I took step back and figure out why not put features as a section of supports that has quite good support in the codebase. Based on that idea I included block overrides for features using the following definition of the block:

registerBlockType( 'my/block', {
    supports: {
        __experimentalFeatures: {
            typography: {
                dropCap: true,
            }
        },
    },
    // ... everything else
} );

Open questions

There is still this remaining question whether we need to define default settings in WordPress core that can be overridden by themes on the server using JSON format. An alternative approach would be to use PHP array or move that to the client in the getSettings implementation.

How has this been tested?

First, I ensured that drop cap functionality still works for the Paragraph block as before.

I also manually modified dropCap flag in Paragraph block by modifying the supports field to ensure it disables the feature when it is set to false. I also removed the section for Paragraph block and tested that global settings toggle the features properly.

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • [N/A] My code follows the accessibility standards.
  • My code has proper inline documentation.
  • [N/A] I've included developer documentation if appropriate.
  • [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

@gziolo gziolo added [Package] Block editor /packages/block-editor [Type] Feature New feature to highlight in changelogs. [Status] In Progress Tracking issues with work in progress labels May 12, 2020
@github-actions
Copy link

github-actions bot commented May 12, 2020

Size Change: +52 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 105 kB +36 B (0%)
build/block-library/index.js 119 kB +9 B (0%)
build/blocks/index.js 48.1 kB +7 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.48 kB 0 B
build/block-directory/style-rtl.css 788 B 0 B
build/block-directory/style.css 788 B 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 7.2 kB 0 B
build/block-library/editor.css 7.2 kB 0 B
build/block-library/style-rtl.css 7.48 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 17.1 kB 0 B
build/components/style.css 17.1 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.11 kB 0 B
build/edit-navigation/index.js 6.63 kB 0 B
build/edit-navigation/style-rtl.css 857 B 0 B
build/edit-navigation/style.css 856 B 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 14 kB 0 B
build/edit-site/style-rtl.css 5.52 kB 0 B
build/edit-site/style.css 5.53 kB 0 B
build/edit-widgets/index.js 8.05 kB 0 B
build/edit-widgets/style-rtl.css 4.59 kB 0 B
build/edit-widgets/style.css 4.59 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.6 kB 0 B
build/editor/style-rtl.css 5.06 kB 0 B
build/editor/style.css 5.06 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.71 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

lib/theme.php Outdated Show resolved Hide resolved
lib/theme.php Outdated Show resolved Hide resolved
@gziolo gziolo self-assigned this May 13, 2020
@gziolo gziolo force-pushed the update/block-editor-features-config branch from a9246d4 to bcfb64f Compare May 13, 2020 16:52
@youknowriad youknowriad added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label May 14, 2020
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I think the idea is that typography.dropCapEnabled may be enabled globally but, for example, disable inside a specific template part.
I'm not sure if we should include __experimentalBlocksConfig and __experimentalBlocksFeatures as settings. I imagine we may have a useGlobalStyles hook that queries the endpoint we will use to save and retrieve the global style.
That hook would be contextual, e.g., it would be aware that a paragraph inside a specific template part may have typography.dropCapEnabled disable while a paragraph not nested may have it enabled.
How to contextualize the global styles is not yet decided, so I guess provided we mark reverting as experimental for now, we can keep the current approach.

@gziolo
Copy link
Member Author

gziolo commented May 15, 2020

I think the idea is that typography.dropCapEnabled may be enabled globally but, for example, disable inside a specific template part.

We want to have a way to give users control for this feature globally and on the block level for start. It's enough to handle. The idea is to offer useEditorFeature hook as the way to resolve it in the block editor so we could apply any type of filtering in the future. This PR illustrates how we could override the global setting for an individual block.

I'm not sure if we should include __experimentalBlocksConfig and __experimentalBlocksFeatures as settings. I imagine we may have a useGlobalStyles hook that queries the endpoint we will use to save and retrieve the global style.

For this PR, it probably makes sense to remove __experimentalBlocksConfig which is something that is still discussed in relation to theme.json defined by the theme rather than Gutenberg itself. I believe that Gutenberg should only provide defaults that are included in __experimentalFeaturesConfig and we might even want to keep those defaults on the client rather than in another theme.json file inside the lib directory.

@gziolo gziolo force-pushed the update/block-editor-features-config branch 2 times, most recently from a9553e8 to 2fb9fb8 Compare May 20, 2020 06:54
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label May 20, 2020
@gziolo gziolo marked this pull request as ready for review May 20, 2020 06:55
@gziolo
Copy link
Member Author

gziolo commented May 22, 2020

We discussed with @youknowriad that it would be nice to document the priorities:

theme.json, block.json... which overrides which... and how to use these

I plan to follow @nosolosw and his work in #22518 and include a section to:

https://github.com/WordPress/gutenberg/blob/38155149ad7cf583d08044fd7654ce5fb1589d1e/docs/designers-developers/developers/themes/theme-json.md

@gziolo gziolo force-pushed the update/block-editor-features-config branch from be46697 to 68643cb Compare May 22, 2020 12:00
@gziolo
Copy link
Member Author

gziolo commented May 22, 2020

I rebased this branch with master and applied all necessary changes. I also renamed lib/theme.php to lib/editor-features.php. Everything should operate as before. I still want to extend the new documentation page that touches theme.json, but it shouldn't be a blocker for this PR.

@gziolo
Copy link
Member Author

gziolo commented May 26, 2020

I still want to extend the new documentation page that touches theme.json, but it shouldn't be a blocker for this PR.

I opened #22622 that is focused on documenting all the changes proposed in this PR.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

👋 This is me thinking out loud about how all these pieces fit together, pardon me if you already know all of this, so I have a link for future reference. For clarity, I'll use core.json to refer to lib/experimental-default-theme.json and theme.json for the one provided by the theme.

So, we currently have:

  • Block settings: persisted in the core/blocks store. There's a blocks.registerBlockType filter in the client to hook into this. This data comes from block.json.

  • Editor settings: persisted in the core/block-editor store. There's a block_editor_settings filter in the server to hook into this. This data comes from PHP (several sources).

And we want:

  1. Block settings:

    1. Default values for supports are taken from block.json.
    2. Via the blocks.registerBlockType client filter, we override the defaults with data taken form the core/block-name subtree (the result of merging core.json & theme.json).
    3. Via the blocks.registerBlockType client filter third-party code can still provide further overrides.
  2. Editor settings:

    1. Default values for editor settings taken from PHP (several sources).
    2. Via the block_editor_settings server filter, we override the defaults with data taken from the global subtree (the result of merging core.json & theme.json).
    3. Via the block_editor_settings server filter, third-party code can still provide further overrides.

The useEditorFeature component merges the block & editor preferences in this way: if they exist, block settings take precedence over editor settings.

This PR implements 2.2, but we still need to implement 1.2 (it's fine if we do this in a follow-up PR).

*/
function gutenberg_experimental_get_editor_features_config() {
$empty_config = array();
$config_path = __DIR__ . '/experimental-default-theme.json';
Copy link
Member

Choose a reason for hiding this comment

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

As you suggested, I'm thinking that we can move the theme.json (both core and theme) to PHP. This also solves one potential code path in which we return the empty config without any values. We can do this in a follow-up as we also have to do it for global styles.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's take care of it separately. I'm still unsure whether this JSON file shouldn't be used as a default for @wordpress/block-editor settings object:

https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/store/defaults.js

There is also this question whether should I put some defaults there as well for features 😅

@gziolo
Copy link
Member Author

gziolo commented May 26, 2020

The useEditorFeature component merges the block & editor preferences in this way: if they exist, block settings take precedence over editor settings.

Correct.

This PR implements 2.2, but we still need to implement 1.2 (it's fine if we do this in a follow-up PR).

At the moment we don't entirely implement 2.2, because theme.json (experimental-theme.json) isn't loaded, we load only features from global section of core.json (lib/experimental-default-theme.json).

In general, we need to decide separately how theme.json fits in this picture. For simplicity, it was removed from the scope in the final proposal.

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

I'm approving this, so we can iterate on related things.

I'd still like if we could remove the extra default parameter added to useEditorFeature. It's not necessary for this feature just yet (if we remove it, this still works as expected). Once a few of the follow-up tasks are ready I think we won't need it, and having it now makes it more difficult to remove later.

These are the follow-up tasks that I see:

  • Editor Defaults: take & merge data from the global subtree of the theme.json (now it only takes core defaults).
  • Block Defaults: expand to this area. Ex: that a theme can disable the drop cap for the paragraph block from theme.json.
  • See how we can consolidate lib/experimental-default-theme.json and block-editor/src/store/defaults.js.
  • Document (can happen when a few more of the above things are ready).

@gziolo gziolo force-pushed the update/block-editor-features-config branch from 6e1fde5 to 00eac25 Compare May 27, 2020 05:32
@gziolo
Copy link
Member Author

gziolo commented May 27, 2020

Thank you for all feedback. I will open follow-up tasks shortly.

Document (can happen when a few more of the above things are ready).

It's partially documented in #22622 and we should continue adding new revisions as we build the whole experience.

@oandregal oandregal added [Type] Enhancement A suggestion for improvement. and removed [Type] Feature New feature to highlight in changelogs. labels Jun 8, 2020
@ellatrix ellatrix mentioned this pull request Jun 16, 2020
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants