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

Allow register/unregister block style variations from theme.json #49827

Closed
wants to merge 3 commits into from

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 14, 2023

NO IMPLEMENTATION YET, GATHERING THOUGHTS ABOUT THE API.

Related #49602

What?

This PR explores the ability to register/unregister block style variations from theme.json.

Why?

How?

{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": {
          "plain": false, // UNREGISTER
          "large": { // REGISTER
            "label": "Large", // Needs to be translated, see theme-i18n.json
            "isDefault": true
          }
        }
      }
    }
  }
}

Status

Testing Instructions

This is not working.

@oandregal oandregal self-assigned this Apr 14, 2023
@oandregal oandregal added [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 labels Apr 14, 2023
@oandregal oandregal requested a review from gziolo April 14, 2023 12:26
@oandregal
Copy link
Member Author

As per the plans at #49602 it sounds like this is something we need. I wanted to gather thoughts on API and challenges before any implementation.

@oandregal
Copy link
Member Author

oandregal commented Apr 14, 2023

Variations as an array:

  • It follows the preset format, but it's different from the one used in styles.blocks.blockName.variations.
  • unregistering is weird. We could consider having a different field for unregistering. For example settings.blocks.core/quote.variationsToUnregister.
  • Internationalization just works using the theme-i18n.json spec. See below.
{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": [
          { "name": "plain", "unregister": true }, // UNREGISTER via new field?
          { "name": "large", "label": "Large", "inlineStyle": "", "styleHandle": "", "isDefault": true }
        ]
      }
    }
  }
}

Variations as an object:

  • It follows the existing format used in styles.blocks.blockName.variations, but differs from the preset format.
  • The unregister operation is more ergonomic.
  • Internationalization just works using the theme-i18n.json spec. See below.
{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": {
          "plain": false, // UNREGISTER
          "large": { // REGISTER
            "label": "Large", // Needs to be translated, see theme-i18n.json
            "inlineStyle": "",
            "styleHandle": "",
            "isDefault": true
          }
        }
      }
    }
  }
}

@oandregal
Copy link
Member Author

It looks like any of the two options above will be internationalized without requiring any code changes 🥳 It's a matter of deciding which one has better ergonomics.


Tested internationalization by:

  • Adding metadata to tell which strings to translate. Update lib/theme-i18n.json to contain the following snippet under settings.blocks.*:
"variations": {
    "*":{
        "label": "Style variation name"
    }
}
  • Adding strings to translate. Update lib/theme.json to contain the following snippet under settings.blocks:
"core/quote": {
    "variations": [
        {
            "name": "large",
            "label": "Large using array",
            "inlineStyle": "",
            "styleHandle": "",
            "isDefault": true
        }
    ]
},
"core/paragraph": {
    "variations": {
        "large": {
            "label": "Large using object",
            "inlineStyle": "",
            "styleHandle": "",
            "isDefault": true
        },
        "variationToUnregister": false
    }
}
  • Verify that the strings are taken into consideration. Open your log and paste the following somewhere in a PHP file:
add_filter( 'gettext_with_context', function ( $translation, $text, $context, $domain ) {
	if ( 'default' === $domain && 'Style variation name' === $context ) {
		error_log( $text );
	}
	return $translation;
}, 10, 4 );

the expected result is that the log contains the strings to translate added in lib/theme.json:

[14-Apr-2023 15:36:57 UTC] Large using array
[14-Apr-2023 15:36:57 UTC] Large using object
[14-Apr-2023 15:36:57 UTC] Large using array
[14-Apr-2023 15:36:57 UTC] Large using object
[14-Apr-2023 15:36:57 UTC] Large using array
[14-Apr-2023 15:36:57 UTC] Large using object
[14-Apr-2023 15:36:57 UTC] Large using array
[14-Apr-2023 15:36:57 UTC] Large using object

@oandregal
Copy link
Member Author

Given internationalization is not a blocker, I'm inclined towards the API that declares variations as an object:

{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": {
          "plain": false, // UNREGISTER
          "large": { // REGISTER
            "label": "Large", // Needs to be translated, see theme-i18n.json
            "inlineStyle": "",
            "styleHandle": "",
            "isDefault": true
          }
        }
      }
    }
  }
}

Thoughts @tellthemachines @gziolo @carolinan ?

@gziolo
Copy link
Member

gziolo commented Apr 17, 2023

I think the biggest challenge here would be that both proposed APIs try to solve a similar issue differently than people do it today for all types of settings that list options: duotone, gradients, fonts, colors, shadows, etc. As of today, you need to provide the full list of overrides for existing settings, whereas here we see a try to inject more style variations or remove some that exist. Taking that into account, if we were to create a common language to apply atomic changes to all existing similar settings I would prefer the object-based notation with even more expressiveness:

{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": {
          "plain": false, // UNREGISTER
          "large": { // REGISTER
            "label": "Large", // Needs to be translated, see theme-i18n.json
            "inlineStyle": "",
            "styleHandle": "",
            "isDefault": true
          }
        }
      }
    }
  }
}

Some implementation notes:

  • unregistering could be also expressed as null.
  • changing the default style might be a bit tricky in this format if the style as the default one remains as one of the options

I like the idea overall, this would drastically simplify managing block styles by themes. I would only try to keep the ways of managing systems consistent between different options so here I would also consider replacing all style variations with the list of options expressed as an array like in other places.

@carolinan
Copy link
Contributor

carolinan commented Apr 17, 2023

What will be the difference between
"inlineStyle":
And
"CSS"? (which is under styles > blocks > variations )

@oandregal
Copy link
Member Author

What will be the difference between "inlineStyle" and "CSS"? (which is under styles > blocks > variations )

Good point. Registration doesn't need to provide the CSS, we should use styles.blocks.variations for that. It creates an edge case: what should happen if a variation is registered, but no CSS is provided via styles.block.variations? I lean towards don't register the variation in that case, as the point of this API is "manage" the style variations via theme.json.

What about style_handle? Is it necessary in this API? I'm unsure about how valuable is this, so I lean towards no exposing it either.

@talldan
Copy link
Contributor

talldan commented Apr 18, 2023

I wonder whether variations as the property name might be too confusing, given the existence of a separate block variations API. I realise there might be other reasons to call it that (consistency with theme variations, and just calling it styles might be too vague or similar to other properties).

Another thing is that isDefault has never really worked correctly for those using the registerBlockStyle API. Maybe it's different for the theme.json, as that would be the source of truth?

@tellthemachines
Copy link
Contributor

try to solve a similar issue differently than people do it today for all types of settings that list options: duotone, gradients, fonts, colors, shadows, etc. As of today, you need to provide the full list of overrides for existing settings

I'm not sure I get this @gziolo . When do we need to provide full list of overrides? When specifying styles for individual blocks in theme.json it works to just set a text color, or a font size, without mentioning all the other color or typography settings. I'd expect the block style variations to work similarly.

From the two options, I'm more inclined towards the object, as it reads better and doesn't require us to change the current API for editing existing block style variations from theme.json. (I don't think consistency with presets is required because presets have more of a site-wide design token function, whereas these are block-level settings.)

Some questions to help think this through:

  • what would inlineStyle actually do in this context?
  • do we need an explicit styleHandle or can we just grab it from the object key?
  • is it weird for the actual styles (e.g. "color": { "background": "hotpink" }) to be at the same level as the more meta settings like label? it would be nice if adding a new style variation followed the same basic structure the same as editing an existing one.

@gziolo
Copy link
Member

gziolo commented Apr 18, 2023

I'm not sure I get this @gziolo . When do we need to provide full list of overrides? When specifying styles for individual blocks in theme.json it works to just set a text color, or a font size, without mentioning all the other color or typography settings. I'd expect the block style variations to work similarly.

Example for font sizes in Twenty Twenty-Three theme:

https://github.com/WordPress/wordpress-develop/blob/b24f990f4f7789cee65628ef090a9e68a2798b5b/src/wp-content/themes/twentytwentythree/theme.json#L249L288

The same setting in WordPress core:

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/theme.json#L375L396

I'm only saying that I'm not aware of existing mechanisms for modifying the default list by adding, overriding, or deleting existing values. So either I miss something obvious or you mentioned a different type of setting.

@gziolo
Copy link
Member

gziolo commented Apr 18, 2023

I have just learned about #49826 where the new feature for injecting styles into variations got documented. It uses an object with keys represented as a style variation name:

https://github.com/WordPress/gutenberg/blob/70744b3684a51082608a4344aa3a1a6fa24f78bb/docs/how-to-guides/themes/theme-json.md?plain=1#L1102L1109

It's quite interesting that you want to split it between two sections: styles and settings.

@tellthemachines
Copy link
Contributor

I'm only saying that I'm not aware of existing mechanisms for modifying the default list by adding, overriding, or deleting existing values. So either I miss something obvious or you mentioned a different type of setting.

Got it, yes, you mean the preset lists! I was thinking of the design tools/block supports settings for individual blocks. In my mind style variations are closer to block styles than to presets.

@carolinan
Copy link
Contributor

I wonder whether variations as the property name might be too confusing,

I have to re-read anything that includes custom, variation, or style multiple times to try to understand what is really meant.
Sometimes it is a theme.json variation inside a styles folder, sometimes its a block variation 🧠 ... both unrelated to custom block styles.
I think "the ship has sailed", I am not sure how we can rename features now.

@talldan
Copy link
Contributor

talldan commented Apr 18, 2023

I think "the ship has sailed", I am not sure how we can rename features now.

This particular feature is called 'block styles' in most other places (the current API for it is registerBlockStyle / register_block_style).

I think 'Style variations' is only an internal name, and one that has been avoided since block variations were introduced.

@carolinan
Copy link
Contributor

The panel label under Site Editor > Styles > Blocks is Style variations

@talldan
Copy link
Contributor

talldan commented Apr 18, 2023

The panel label under Site Editor > Styles > Blocks is Style variations

I'm talking about the naming used in the code, which doesn't have to match what's in the UI (though it's a bonus if it does). It's good to have a consistent API, and considering a name (block styles) was already chosen, my thinking was to keep it consistent.

All the developer documentation also refers to it as block styles.

When I say 'internal', I mean that some gutenberg contributors call it that.

@oandregal
Copy link
Member Author

oandregal commented Apr 18, 2023

I wonder whether variations as the property name might be too confusing, given the existence of a separate block variations API. I realise there might be other reasons to call it that (consistency with theme variations, and just calling it styles might be too vague or similar to other properties).

So, naming. This ship already sailed: block style variations landed in 6.2 as styles.blocks.variations. Unless we strongly want to change the name, which means updating the theme.json version to v3, we need to stick to what we have.

It's quite interesting that you want to split it between two sections: styles and settings.

Right. It's somehow the same as naming: we could have added block styles variations in a single place, in settings. That ship already sailed: styles.blocks.variations is already part of the API since 6.2. The best we can do now is not duplicating in settings.blocks.variations what is already defined via styles.blocks.variations, so I agree with Carolina that inline_styles can be left out.

Taking a step back to visualize the whole proposal so far:

{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": { /* PROPOSAL TO LAND IN 6.3 */
          "plain": "false or null to unregister"
          "newVariationToRegister": {
            "label": "Needs to be translated, via theme-i18n.json"
            "styleHandle": "what is this for? is it useful?",
            "isDefault": true
          }
        }
      }
    }
  }
  "styles": {
    "blocks": {
      "core/quote":{
        "variations": { /* THIS ALREADY LANDED IN 6.2 */
          "color": {},
          "typography": {},
          "...": {}
        }
      }
    }
  }
}

Grzegorz brought up that this differs from how presets work (they use an array instead of an object). One alternative to the above would be having separate keys for registering and unregistering style variations. For example:

{
  "settings": {
    "blocks": {
      "core/quote": {
        "variations": [ /* PROPOSAL TO LAND IN 6.3 */
          { "name": "new-variation", "label": "New variation", "styleHandle": "...", "isDefault": true }
        ],
        "variationsToUnregister": [ "plain" ]
      }
    }
  }
  "styles": {
    "blocks": {
      "core/quote":{
        "variations": { /* THIS ALREADY LANDED IN 6.2 */
          "color": {},
          "typography": {},
          "...": {}
        }
      }
    }
  }
}

@gziolo
Copy link
Member

gziolo commented Apr 18, 2023

Excellent summary @oandregal. I'm happy with both approaches now that I see full picture 👍🏻

@tellthemachines
Copy link
Contributor

I have to re-read anything that includes custom, variation, or style multiple times to try to understand what is really meant

Same here 😅

I think 'Style variations' is only an internal name, and one that has been avoided since block variations were introduced.

I went with "variations" when making them editable in global styles because they're called that in #44417, and because they're analogous to theme style variations (in both cases we can select from a bunch of customisations which we can then further customise).

It's a shame that block variations have the same name, as they behave differently from both these cases. On the other hand, block variations are invisible as such from a UI perspective; they're only known as variations in the block API.

I also think using just "styles" would be super confusing in theme.json because we'd end up with a nested structure like:

"styles": {
    "blocks": {
      "core/button": {
        "styles": {
          (...)
        } 
      }
    }
  }

I'm not sure how clear it would be to call them "styles" in the UI either, it's already confusing enough to have global styles referred to as just "styles", and now we have "styles" and "settings" inside the "settings" sidebar too 😄

Screenshot 2023-04-19 at 2 31 55 pm

@tellthemachines
Copy link
Contributor

Just a note that the existing API for editing variations has the same shape as the proposed object API:

 "core/quote":{
        "variations": { /* THIS ALREADY LANDED IN 6.2 */
          "plain": {
              "color": {},
              "typography": {}
           }
        }
      }

as it's not clear from the example above. Which makes me wonder if the new API would be like:

"core/quote": {
        "variations": { /* PROPOSAL TO LAND IN 6.3 */
          "plain": "false or null to unregister"
          "newVariationToRegister": {
            "label": "Needs to be translated, via theme-i18n.json"
            "styleHandle": "what is this for? is it useful?",
            "isDefault": true,
            "color": {},
            "typography": {}
          }
        }
      }

will the styles sit at the same level as the metadata?

@carolinan
Copy link
Contributor

I think the style handle should be kept. I don't find it so useful personally but I think developers will expect all the different registrations method to match.
Alternatively keep it out and add it if it is requested...

@oandregal
Copy link
Member Author

will the styles sit at the same level as the metadata?

My current view is that styles should be provided via styles.blocks.blockName.variations. settings.blocks.blockName.variations should only contain metadata (which also won't support inline_style).

An alternative would be to allow providing structured styles in both places, though I'd recommend against that.

What do you think @tellthemachines?

@oandregal
Copy link
Member Author

So, I've reviewed the docs, the PR that first introduced block style variations and the PR that updated them to consider loading styles per-block.

It seems to me that we don't need inline_styles or style_handle when style variations are defined via theme.json. As I understand them, these mechanisms were created to enqueue styles: either inline or using a separate stylesheet. Because style variations defined via theme.json will be already enqueued using the global styles stylesheet, this API doesn't need those fields.

@oandregal
Copy link
Member Author

OK. I've updated the PR description with the current API proposal and other notes. I'll wait one day or two before starting implementation in case there is any other feedback to address.

@tellthemachines
Copy link
Contributor

settings.blocks.blockName.variations should only contain metadata (which also won't support inline_style).

Ah, got it, I somehow missed that metadata was nested under settings 😅
Yeah that makes sense, better to not have styles in the settings object.

@conatus
Copy link

conatus commented Jun 26, 2023

Great to see this coming in. Just out of interest, how would one go about implementing this today without this work being done and merged in?

@oandregal
Copy link
Member Author

Closing this one, because I won't have time to implement this myself.

@oandregal oandregal closed this Oct 31, 2023
@oandregal oandregal deleted the try/register-variations-from-theme-json branch October 31, 2023 16:45
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 [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants