search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.32k stars 4.12k forks source link

Next iteration of block supports mechanism #28913

Closed oandregal closed 3 years ago

oandregal commented 3 years ago

Part of https://github.com/WordPress/gutenberg/issues/20331 Implementing this would enable some interactions expressed in https://github.com/WordPress/gutenberg/issues/26313 Kudos to @mtias @youknowriad for helping to identify the next steps for this.

What we have now

The block support mechanism allows block authors to easily make their blocks customizable via adding support for style properties: font size, color, etc.

Under the hood, this mechanism does three things:

  1. Bounds UI controls to the block sidebar & toolbar.
  2. Creates an implicit attribute for the block.
  3. Casts the implicit attribute to some DOM characteristic of the root element of the block (a style attribute or a new class).

There are some use cases that aren't absorbed by this mechanism, including:

The struggle boils down to the third point above: cast the implicit attribute to some DOM characteristic to the root node.

Next steps

A path to explore is allowing block authors a finer-grained control of how block supports work. We want to retain the bounding of UI controls to specific attributes so that block authors don't need to fiddle with controls and attribute flow. Hence, we'd allow declaring support in a way that the framework will handle rendering the UI and keeping the attribute in sync (1 and 2 in the list above) but the block author is responsible for casting the attribute on the right element (3).

Related to this fits into the global styles mechanism. Currently, blocks declare a single selector for all their style properties, so when theme authors do:

"styles":
  "core/paragraph": {
    "color": {
      "background": "red"
    },
    "typography": {
      "fontSize": "23px",
    }
}

this is transformed to:

p {
  background-color: red;
  font-size: 23px;
}

There are cases in which block selectors will need to be bounded to the support properties instead of bounded to the block, given that they may apply to different inner elements. If we have the selector bounded to a block support, the framework could also try to absorb the casting (3 in the list above) client-side.

Tasks

Adapt a single property and make one block use it: update how colors work in the social links block.

Framework implementation

Migrate these blocks & properties to the block supports system:

oandregal commented 3 years ago

I've started working on this as per https://github.com/WordPress/gutenberg/pull/29142 Also added an initial list of tasks to implement.

oandregal commented 3 years ago

@mkaz @jasmussen I think you were involved in the social links block. Wanted to give you a heads-up that I've started to work on this issue and I'm targeting the colors of the social links block as a demo for this, so expect some PRs for that block soon.

jasmussen commented 3 years ago

I'd be very happy to review that! Awesome.

oandregal commented 3 years ago

Landed #29142 (to skip serialization) and #29155 is ready for review (take labels for controls from block supports).

scruffian commented 3 years ago

Using __experimentalSelector as the mechanism for determining which child block the supports should apply to might be a bit limiting. For example, in Social Links we want to apply the colors to the child block, but the alignment would apply to the parent block. What do you think about declaring a selector per supports e.g:

    "supports": {
        "align": [
            "left",
            "center",
            "right"
        ],
        "anchor": true,
        "color": {
            "background": true,
            "__experimentalSkipSerialization": true,
            "__experimentalSelector": ".wp-social-link"
        },
        "font-family: {
            "__experimentalSelector": ".wp-social-link a"
        }
    }
oandregal commented 3 years ago

@scruffian yes! That's what I meant with: There are cases in which block selectors will need to be bounded to the support properties instead of bounded to the block, given that they may apply to different inner elements. :sweat_smile: Happy you all were thinking along the same lines. I've already started working on this so expect to have something in your review queues in the coming days.

oandregal commented 3 years ago

Update on this task: __skipSerialization is now deployed to the buttons block and the color hook. The core of the task is done, although there are related tasks to complete (see "misc" and "fixing specificity" sections).

I added an initial list of blocks that could benefit from this and also the hooks for which __skipSerialization remains to be implemented. cc @aaronrobertshaw @apeatling @aristath @scruffian as you've worked in this area and have related PRs for this, in case you can spare some time to help to implement the rest of the hooks and blocks.

oandregal commented 3 years ago

My next step is to fix https://github.com/WordPress/gutenberg/issues/29381

oandregal commented 3 years ago

There's a PR for https://github.com/WordPress/gutenberg/issues/29381 at https://github.com/WordPress/gutenberg/pull/29533

oandregal commented 3 years ago

I've updated the issue with the ongoing tasks and their owners. Please, feel free to add yourself/edit/etc as things evolve (also leaving a comment so subscribers of the task know there are changes).

oandregal commented 3 years ago

@ockham has landed https://github.com/WordPress/gutenberg/pull/30035 (skip serialization for border)

oandregal commented 3 years ago

There's two ongoing PRs to use skip serialization for border: search block by @aaronrobertshaw #30227 and button block https://github.com/WordPress/gutenberg/pull/30194 by @ockham

oandregal commented 3 years ago

Labels for UI controls: closed https://github.com/WordPress/gutenberg/pull/29155#issuecomment-806583346 in favor of https://github.com/WordPress/gutenberg/pull/30075

jorgefilipecosta commented 3 years ago

Re-sharing a comment on https://github.com/WordPress/gutenberg/pull/30879#pullrequestreview-641302448 here for better visibility as this issue seems to include other support mechanism changes:

Our block.json supports API right now has inconsistencies in how we handle typography vs colors:

        "color": {
            "gradients": true,
            "link": true
        },
        "fontSize": true,
        "lineHeight": true,

It is also inconsistent with theme.json given that there we have:

            "typography": {
                "fontSize": "value",
                "lineHeight": "value",
            }

I guess we can change the shape of block.json and be back-compatible. We can have a back-compatibility layer that checks if things like fontSize and lineHeight are present in the data structure and if yes nests them under typography. Our code excluding the back-compatibility layer would only rely on the new structure. The shape change would be done in another PR. I'm not sure if a change like this would require a new "apiVersion", I guess it is something minor that does not requires it, but I'm cc'ing @gziolo in case he has some thoughts.

gziolo commented 3 years ago

Re-sharing from https://github.com/WordPress/gutenberg/pull/30879#issuecomment-824615316 my response to the question from @jorgefilipecosta:

The shape change would be done in another PR. I'm not sure if a change like this would require a new "apiVersion", I guess it is something minor that does not requires it, but I'm cc'ing @gziolo in case he has some thoughts.

It's already documented at https://developer.wordpress.org/block-editor/reference-guides/block-api/block-supports/, but it would be good to validate whether it was released in the previous major version of WordPress. If not then we can simply change it and keep the backward compatibility layer only in the plugin for a few more releases.

In general, the question goes to @oandregal how much supports in block.json should align with theme.json as it doesn't necessarily translate one to one. Conceptually, I would be in favor of using the same naming and grouping in both places to make it consistent. If they are also grouped in the UI in the same way, then it adds another reason to make the extra efforts to bring harmony how all that fits together.

Concluding, if this is only a simple mapping layer that would have to map some properties from their deprecated names, then we should definitely go for it. In the documentation, we should simply remove old variants in favor of the refined approach.

oandregal commented 3 years ago

I think we should either group those things together as in theme.json or have them all unnested (do not have the color intermediate key) ― the in-between state we have now is confusing. For context https://github.com/WordPress/gutenberg/issues/22887 https://github.com/WordPress/gutenberg/pull/24320

A question I have (that's why I'm posting here instead of in the font-size PR) is whether __experimentalSkipSerialization should be a top-level key of supports and not having one per "area" (color, typography). This is: whether a block opts out from serialization entirely of per property. Per property is more flexible but I'm curious about what people think about doing it per block and so avoiding all those skip flags everywhere ― case in point: the button block is the first that has support for many properties and needs to skip serialization and, as a result, what happens is that it needs to use that flag in multiple places.

aaronrobertshaw commented 3 years ago

I think we should either group those things together as in theme.json or have them all unnested

I tend to prefer the idea of grouping theme together as per theme.json. If nothing else it's neater and allows opting into a set of features via a single support flag e.g. "__experimentalBorder": true.

A question I have (that's why I'm posting here instead of in the font-size PR) is whether __experimentalSkipSerialization should be a top-level key of supports and not having one per "area" (color, typography).

Skipping serialization per "feature set" or "area" would get my vote.

As more blocks adopt the various block support features, I think the extra flexibility will pay off. It will cut down on the manual retrieval and application of classes and styles. In a lot of cases, applying the block classes and styles on the wrapper combined with a CSS style to inherit values if needed, gets the job done cleanly. That leaves only the cases that truly need to skip serialization to manually apply things to the inner elements.

If the idea is to reduce the number of flags in the config in certain cases, could we get the best of both worlds by supporting a top level __experimentalSkipAllSerialization flag in addition to the "per property" ones?

In a block that wants to skip all serialization it is one simple flag. For one that needs to keep its border styles on the wrapper element but apply colors selectively to an inner element, it can skip the colors serialization and manage only those manually.

aaronrobertshaw commented 3 years ago

The initial switch to block support colors for the Table block has been merged and a follow up issue created to track the requested iterations

jorgefilipecosta commented 3 years ago

Skipping serialization per "feature set" or "area" would get my vote.

Yes, I think serialization per area is a good tradeoff in what it allows to do and the effort required to disable each serialization.

I'm not sure if experimentalSkipSerialization is the right name for the flag that disables the output of the styles. On server-side rendered blocks, there is no serialization, so one may think this flag does nothing, but that's not the case; the flag disables the dynamic server-side mechanism that outputs the styles. During a chat with @mcsf, he referred experimentalUIOnly as a possible name I agree experimentalUIOnly may be a good option, and experimentalSkipStylesOutput could also be another option.

jorgefilipecosta commented 3 years ago

I checked the "Color for navigation" task because it seems the navigation block is already using the color supports mechanism.

jorgefilipecosta commented 3 years ago

I'm not sure if experimentalSkipSerialization is the right name for the flag that disables the output of the styles. On server-side rendered blocks there is no serialization so one may think this flag does nothing but that's not the case the flag disables the dynamic server-side mechanism that outputs the styles. During a chat with @mcsf he referred experimentalUIOnly as a possible name I agree experimentalUIOnly may be a good option and experimentalSkipStylesOutput could also be another option.

ockham commented 3 years ago

__experimentalUIOnly reads a bit confusing to me -- while I know that the __experimental is the prefix (and I thus need to read it as "experimental flag 'UI only'"), it's possible to read it as "'experimental UI' only" -- only show the experimental UI (but not the non-experimental one?).

__experimentalSkipStylesOutput sounds better to me (tho in the font size case, we also have a class output).

aaronrobertshaw commented 3 years ago

Personally, I find the __experimentalUIOnly option more confusing and possibly inaccurate than __experimentalSkipSerialization. Once the styles and classes are applied to inner elements, it's no longer just the UI.

I agree with the point regarding there being no serialization on the server-side rendered blocks though.

__experimentalSkipStylesOutput sounds better to me (tho in the font size case, we also have a class output).

The same goes for background, text and border colors.

The flag name is getting rather long but maybe something along the lines of __experimentalSkipPresentationOutput?

stokesman commented 3 years ago

__experimentalUnstyled? I think that's general enough it could be expected to cover class output as well.

oandregal commented 3 years ago

All properties have gotten the _skipSerialization flag, so I'm going to close this one. We can still migrate blocks iteratively, do follow-ups, etc ― but for the purpose of tracking this work, we're done.

gziolo commented 2 years ago

Inspired by the discussion with @luisherranz and @c4rl0sbr4v0 in #35473, I wanted to summarize my understanding of how the experimental flag __experimentalSkipSerialization helps to control which class names and styles should get injected into the block wrapper.

Code examples from the Button block present how you can use __experimentalSkipSerialization flag to tell that a given feature shouldn't be applied to the block wrapper:

https://github.com/WordPress/gutenberg/blob/b55cede14c3a6f0ed797d73e676c9fc504fd58d4/packages/block-library/src/button/block.json#L60-L61

https://github.com/WordPress/gutenberg/blob/b55cede14c3a6f0ed797d73e676c9fc504fd58d4/packages/block-library/src/button/block.json#L69-L70

https://github.com/WordPress/gutenberg/blob/b55cede14c3a6f0ed797d73e676c9fc504fd58d4/packages/block-library/src/button/block.json#L76-L78

There are corresponding hooks that let you read those skipped features and apply them to any other nested HTML element:

https://github.com/WordPress/gutenberg/blob/b55cede14c3a6f0ed797d73e676c9fc504fd58d4/packages/block-library/src/button/edit.js#L123-L125

They need to be mapped to class names and styles:

https://github.com/WordPress/gutenberg/blob/b55cede14c3a6f0ed797d73e676c9fc504fd58d4/packages/block-library/src/button/edit.js#L170-L185

I'm not sure how it could be translated to code in render_callback on PHP side though.

michalczaplinski commented 2 years ago

Inspired by the discussion with @luisherranz and @c4rl0sbr4v0 in #35473, I wanted to summarize my understanding of how the experimental flag __experimentalSkipSerialization helps to control which class names and styles should get injected into the block wrapper.

Thank you @gziolo that helped me understand it quickly after looking around the various issues without success!

cbravobernal commented 2 years ago

Here we have an interation of applying different styles to different components both in JS and in PHP.

https://github.com/WordPress/gutenberg/pull/36322/files