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

Behaviors - Lightbox: Update theme.json schema #51156

Merged
merged 18 commits into from
Jul 24, 2023

Conversation

cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Jun 1, 2023

What?

Updating the theme.json schema with the new values available for Behaviors UI and the Lightbox feature.

As Behaviors UI and the Lightbox feature can be enabled and disabled through the theme.json file. Updating the schemas should facilitate its use for Theme developers.

Important: ⚠️ ⚠️ Do not merge while behaviors and lightbox are behind the experimental flag.

@cbravobernal cbravobernal changed the title Update/theme json schema behaviors Behaviors - Lightbox: Update theme.json schema Jun 1, 2023
@cbravobernal cbravobernal self-assigned this Jun 1, 2023
@cbravobernal cbravobernal added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Code Quality Issues or PRs that relate to code quality labels Jun 1, 2023
@cbravobernal cbravobernal marked this pull request as ready for review June 1, 2023 11:29
Copy link
Contributor

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this 🙂 I just have one comment:

It seems that when defining "settings.blocks.core/image.behaviors.lightbox` still complains in Visual Studio:

Screenshot 2023-06-01 at 17 26 31

I am not familiar with schemas but, if we are using allOf in the code (here), should we include "behaviors" here as well? If I am not mistaken, that would allow other blocks to include the "behaviors" property, but that is something that is already happening. For example, I can define "settings.blocks.core/post-date.border" which isn't supported.

I guess we could try using anyOf as well, but that seems to allow any property.

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

I guess there's a need to be specific until the behavior API is extended to more blocks. I think we could still create a behaviors properties definition, similar to settingsPropertiesComplete et al so that our future selves, and other devs, know where to add them.

It seems that when defining "settings.blocks.core/image.behaviors.lightbox` still complains in Visual Studio:

Is your theme's theme.json pointing to "$schema": "https://schemas.wp.org/trunk/theme.json",?

You can point it to the local copy. See the README

@@ -487,7 +487,7 @@
"type": "string"
},
"fluid": {
"description": "Specifics the minimum and maximum font size value of a fluid font size. Set to `false` to bypass fluid calculations and use the static `size` value.",
"description": "Specifies the minimum and maximum font size value of a fluid font size. Set to `false` to bypass fluid calculations and use the static `size` value.",
Copy link
Member

Choose a reason for hiding this comment

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

Ha, good one! I spotted this yesterday and fixed. 👍

schemas/json/theme.json Show resolved Hide resolved
@t-hamano
Copy link
Contributor

t-hamano commented Jun 2, 2023

I have tested with the following file, which references this PR as the schema URL.

{
	"$schema": "https://github.com/raw/WordPress/gutenberg/update/theme-json-schema-behaviors/schemas/json/theme.json",
	"version": 2
}

The description of the root level behaviors puzzled me.

behaviors

This is because the default type of value is an object, even though it is described as "an array of blocks."

How about moving this description to behaviors.blocks? In that case, I think we need a separate description for the root level behaviors.


It would be nice to have validation in the behaviors.blocks property to check if the block name is correct, like settings.blocks or settings.blocks.

wrong-block-name


The behaviors.blocks.core/image allows all properties and values. It may be better to use additionalProperties": false" and not allow any properties other than lightbox. This is also the case for settings.blocks.core/image.behaviors.

additionalProperties


The settings.blocks.core/image property suggests behaviors as a definable property. However, the defined property causes an error as not allowed.

not-allowed

@SantosGuillamot
Copy link
Contributor

Is your theme's theme.json pointing to "$schema": "https://schemas.wp.org/trunk/theme.json",?

I was testing it pointing to a local file and making changes to it seems to work 🤷 Thank you anyway for the information! 🙂

@michalczaplinski michalczaplinski force-pushed the update/theme-json-schema-behaviors branch from f8cc3d2 to a9e379f Compare July 6, 2023 15:24
@michalczaplinski
Copy link
Contributor

I've updated the PR with the correct types for the Lightbox animations. I think this is ready now, but we should wait until removing the experimental flag as mentioned by Carlos.

Just noting here that we'll also need to change the types for block.json now that we're using the block supports.

@cbravobernal cbravobernal marked this pull request as ready for review July 24, 2023 12:25
@michalczaplinski michalczaplinski merged commit 961d8ec into trunk Jul 24, 2023
48 checks passed
@michalczaplinski michalczaplinski deleted the update/theme-json-schema-behaviors branch July 24, 2023 13:42
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants