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

Next iteration of block supports mechanism #28913

Closed
17 of 20 tasks
oandregal opened this issue Feb 10, 2021 · 28 comments
Closed
17 of 20 tasks

Next iteration of block supports mechanism #28913

oandregal opened this issue Feb 10, 2021 · 28 comments
Labels
[Feature] Block API API that allows to express the block paradigm. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json

Comments

@oandregal
Copy link
Member

oandregal commented Feb 10, 2021

Part of #20331
Implementing this would enable some interactions expressed in #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:

  • social links block (color, size)
  • buttons (existing div for backward compatibility)
  • search block (add support for border)

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
Copy link
Member Author

I've started working on this as per #29142 Also added an initial list of tasks to implement.

@oandregal
Copy link
Member Author

@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
Copy link
Contributor

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

@oandregal
Copy link
Member Author

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

@scruffian
Copy link
Contributor

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
Copy link
Member Author

@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. 😅 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
Copy link
Member Author

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
Copy link
Member Author

My next step is to fix #29381

@oandregal
Copy link
Member Author

There's a PR for #29381 at #29533

@oandregal
Copy link
Member Author

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).

@jorgefilipecosta
Copy link
Member

Re-sharing a comment on #30879 (review) 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
Copy link
Member

gziolo commented Apr 22, 2021

Re-sharing from #30879 (comment) 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
Copy link
Member Author

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 #22887 #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.

@oandregal oandregal removed their assignment Apr 26, 2021
@aaronrobertshaw
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

jorgefilipecosta commented Apr 29, 2021

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
Copy link
Member

jorgefilipecosta commented Apr 29, 2021

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

@jorgefilipecosta
Copy link
Member

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
Copy link
Contributor

ockham commented Apr 29, 2021

__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
Copy link
Contributor

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
Copy link
Contributor

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

@oandregal
Copy link
Member Author

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
Copy link
Member

gziolo commented Nov 3, 2021

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:

"color": {
"__experimentalSkipSerialization": true,

"spacing": {
"__experimentalSkipSerialization": true,

"__experimentalBorder": {
"radius": true,
"__experimentalSkipSerialization": true

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

const borderProps = useBorderProps( attributes );
const colorProps = useColorProps( attributes );
const spacingProps = useSpacingProps( attributes );

They need to be mapped to class names and styles:

className={ classnames(
className,
'wp-block-button__link',
colorProps.className,
borderProps.className,
{
// For backwards compatibility add style that isn't
// provided via block support.
'no-border-radius': style?.border?.radius === 0,
}
) }
style={ {
...borderProps.style,
...colorProps.style,
...spacingProps.style,
} }

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

@michalczaplinski
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json
Projects
No open projects
Development

No branches or pull requests

10 participants