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 the style property of block.json to be an array and add support for object-based block styles #2853

Closed

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Jun 22, 2022

What problem does this PR solve?

In WordPress/gutenberg#34180 we moved block CSS to blockJson.supports.__experimentalStyle. This PR moves it under blockJson.style at the root level. The goal is to only have a single location where styles are defined.

Block.json before this PR:

{
	"supports":  {
		"__experimentalStyle": {
			"border": {			
				"color": {
					"text": "#fff",
					"background": "#32373c"
				}
			}
		}
	},
	"style":  "wp-block-button"
}

Block.json after this PR:

{
	"style":  [ "wp-block-button", {
		"border": {			
			"color": {
				"text": "#fff",
				"background": "#32373c"
			}
		}
	}]
}

This PR is an alternative to WordPress/gutenberg#41656

Other considerations

This is a breaking change – any existing code that assumes style to be a single item or an array of strings will throw a notice at best.

Test plan

  1. Apply this PR to your local wordpress-develop
  2. Run npm run build
  3. Apply Move block css from supports > __experimentalStyle to a top level style key in block.json gutenberg#41873 to your local Gutenberg installation
  4. Build Gutenberg
  5. Switch themes to TwentyTwentyTwo
  6. Create a new page
  7. Insert a button, give it some text
  8. Save it, view it on the frontend
  9. Confirm the buttons have a large padding at the bottom

cc @getdave @scruffian @draganescu

Trac ticket: https://core.trac.wordpress.org/ticket/56094

@scruffian
Copy link

This is a breaking change – any existing code that assumes style to be a single item or an array of strings will throw a notice at best.

This seems like a blocker. Can we change it to allow for strings as well?

@adamziel
Copy link
Contributor Author

adamziel commented Jun 24, 2022

This seems like a blocker. Can we change it to allow for strings as well?

@scruffian oh it can be a string. I'm saying it can be either of:

  • A string
  • An array of strings
  • A mixed array of strings and arrays

Therefore, any code that assumes it's just a string or string[] will break.

@adamziel
Copy link
Contributor Author

@scruffian I applied all the feedback and refreshed the related Gutenberg PR. This one is ready for another review!

@gziolo
Copy link
Member

gziolo commented Jun 28, 2022

@adamziel, thank you so much for looking into adding native support for multiple style styles per block type. It's all going in the right direction. It will require a bit more work to propagate changes to other places:

This will include also existing unit tests for both classes. New functionality requires also unit test coverage.

I don't think we consume style from the REST API endpoint in Gutenberg, but we should double-check to be on the safe side.

We should also extend the existing documentation for block.json to include the newly supported format of styles:
https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md#style

What is the reason this patch covers only style but not editorStyle?

src/wp-includes/blocks.php Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented Jun 29, 2022

We will have to put an empty _wp_multiple_block_styles function with the deprecation message, example:

/**
* Inject the block editor assets that need to be loaded into the editor's iframe as an inline script.
*
* @since 5.8.0
* @deprecated 6.0.0
*/
function wp_add_iframed_editor_assets_html() {
_deprecated_function( __FUNCTION__, '6.0.0' );
}

@gziolo
Copy link
Member

gziolo commented Jun 29, 2022

It looks like we have everything covered so it needs a final review and a round of testing with and without the Gutenberg plugin.

@adamziel
Copy link
Contributor Author

adamziel commented Jun 29, 2022

I think we're good now! GitHub CI seems to be down, though, so might be worth waiting until it's up again before merging.

@adamziel adamziel changed the title Allow the style property of block.json to be an array Allow the style property of block.json to be an array and add support for object-based block styles Jun 29, 2022
@adamziel
Copy link
Contributor Author

All the tests passed! Would you please help get this PR approved? CC @gziolo @aristath @scruffian @getdave

Copy link

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

@adamziel
Copy link
Contributor Author

adamziel commented Jul 4, 2022

@gziolo do you think you could merge this one?

@adamziel
Copy link
Contributor Author

adamziel commented Jul 8, 2022

cc @mtias for soundness check – does this change seem reasonable to you?

@gziolo
Copy link
Member

gziolo commented Jul 8, 2022

The example shared in the description looks nice in my opinion as it consolidates several available options. In the future, we could also use the same API borrowed from theme.json for style variations:

{
	"styles": [
		{
			"name": "default",
			"label": "Default",
			"isDefault": true
		},
		{
			"name": "other",
			"label": "Other",
			"settings": {
				"border": {			
					"color": {
						"text": "#000",
						"background": "#ccccc"
					}
				}
			}
		 }
	],
	"style":  [ "wp-block-button", {
		"border": {			
			"color": {
				"text": "#fff",
				"background": "#32373c"
			}
		}
	} ]
}

@gziolo
Copy link
Member

gziolo commented Jul 21, 2022

Let's land WordPress/gutenberg#41873 in the Gutenberg plugin first and give it a try before we include it in WordPress core. Code-wise everything is ready, and it had enough reviewers. We mostly need to validate that the extended API for style in block.json is friendly for developers – it's mostly the question of whether it's intuitive and straightforward to document.

@adamziel
Copy link
Contributor Author

I'm closing this in favor of #3108

@adamziel adamziel closed this Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants