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

useEditorFeature: take block context into account #24416

Merged
merged 2 commits into from
Aug 31, 2020

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Aug 6, 2020

Related #20588
Follows-up #24275
Partially fixes #22657

This PR makes useEditorFeature to take into account the block context data to decide whether a feature should be enabled or not.

This is the current algorithm to decide what value to take:

  1. Is it defined in the corresponding block section (ex: core/paragraph) of the theme's config? If so, take this value.
  2. Is it defined in the corresponding block section (ex: core/paragraph) of the core's config? If so, take this value.
  3. Is it defined in the global section of the theme's config? If so, take this value.
  4. Is it defined in the global section of the core's config? If so, take this value.

Test

Using a theme that supports global styles:

  • test that a feature can be disabled globally
  • test that a feature can be disabled globally but enabled per block
  • test that a feature can be enabled globally but disabled per block

You can use the demo theme at https://github.com/nosolosw/global-styles-theme/pull/12 as a testbed. The expected result is that custom colors are disabled for every block except paragraph.

Follow-up tasks:

  • Make it work for blocks that define many contexts: core/heading (core/heading/h1, */h2, etc), core/post-title, etc.

@oandregal oandregal self-assigned this Aug 6, 2020
@oandregal oandregal added the [Package] Block editor /packages/block-editor label Aug 6, 2020
@oandregal oandregal changed the title Take block context into account to decide whether a feature should be enabled useEditorFeature: take block context into account Aug 6, 2020
@github-actions
Copy link

github-actions bot commented Aug 6, 2020

Size Change: +32 B (0%)

Total Size: 1.17 MB

Filename Size Change
build/block-editor/index.js 126 kB +32 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.44 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.99 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 8.52 kB 0 B
build/block-library/editor.css 8.52 kB 0 B
build/block-library/index.js 136 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.47 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 742 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/blocks/index.js 47.7 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.55 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.48 kB 0 B
build/edit-navigation/index.js 11.7 kB 0 B
build/edit-navigation/style-rtl.css 1.16 kB 0 B
build/edit-navigation/style.css 1.16 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.61 kB 0 B
build/edit-post/style.css 5.61 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 11.9 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.3 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 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 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.52 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 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 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 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.41 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 13.9 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@oandregal
Copy link
Member Author

oandregal commented Aug 6, 2020

I'm thinking we may want to change the algorithm, so the theme can override the block.json defaults. Something like this makes more sense to me:

  1. Is it defined in the block context section (ex: core/paragraph) of the theme's config file? Take this value.
  2. Is it defined in the block context section (ex: core/paragraph) of the core's config file? Take this value.
  3. Is it defined in the support section within block.json? Take this value.
  4. Is it defined in the global context section of the theme's config file? Take this value.
  5. Is it defined in the global context section of the core's config file? Take this value.

@youknowriad
Copy link
Contributor

@nosolosw I was going to suggest this too #24416 (comment) :)

Also, I've been wondering whether the "core's" theme.json file is needed at all? What's its purpose I'm not sure I understand it personally.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I've been thinking more about the priorities here and I think there are very rare use-cases where block.json would actually impact whether a feature is enabled or not, it's more to define that a block supports a feature than to actually enable or disable it.

So I think this PR is good enough to start and we can iterate on the priorities as we explore concrete use-cases.

@oandregal
Copy link
Member Author

Those are questions that I also have:

  1. I'm fine merging this and iterate later. I also want to point out that this PR, as it is, won't allow to override something that a block sets in its block.json. Take the existing example of dropCap for paragraph: there's no way to disable it with this implementation (hence, why I suggested a different algorithm). I wonder what's the value of this mechanism if themes can't override a block.json definition? For this, I'm inclined to start with a different implementation (example) although we iterate later.

  2. Your question about core's theme.json is also one that I have! I tried to bring it up in this thread. Let me share some potential scenarios:

    1. What if we got rid of this file? We'd need a block that represents the global context and is always present => is this something that will happen with FSE work? Related [Explore] Site Block #16998 We'd also need a place to absorb the presets definitions, which, at the moment, don't live in the block.json => for this to happen, the block.json should be translatable (which I think is the goal but hasn't happened yet?).

    2. What if we limited this file to only represent the global context? The only thing we'd need to think about is presets for contexts other than global (which is the same question as before).

@youknowriad
Copy link
Contributor

Yeah for 2, it seems we should keep it for now and see how it evolves. Maybe as you suggest it just becomes the way for Core to define whether a config is opt-in or opt-out.

For 1, you're right that we should change the algorithm in that case.. Here's what I think:

  • If a block doesn't have the "color" support hook in its block.json, it shouldn't be possible for the theme.json to enable/disable that support hook, that's a block decision.
  • If a block does have the "color" support hook in its block.json, the theme.json should be able to enable/disable it.

So block.json defines that a block accepts that feature, and theme.json decides to hide it or show it. And each feature can be opt-in (disabled by default) or opt-out (enabled by default for blocks that support it).

How's that sound?

@oandregal oandregal force-pushed the add/block-support-to-editor-features branch from bcebcb4 to e2c70a1 Compare August 31, 2020 12:00
@oandregal oandregal merged commit 379719f into master Aug 31, 2020
@oandregal oandregal deleted the add/block-support-to-editor-features branch August 31, 2020 14:54
@oandregal oandregal added this to the Gutenberg 8.9 milestone Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Editor: Allow a theme to override settings per block for editor features
2 participants