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

theme.json schema: fix styles.background definition #59595

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Mar 5, 2024

Follow-up #59354

What?

This PR fixes the following two issues regarding the background property, which can only be defined at the top level of the styles property.

Considered a disallowed property:

image

Can be unintentionally defined at block/element level:

image

Why?

The background property is defined as a subproperty of stylesProperties. However, the stylesProperties is also used at the block/element level.

How?

Moved the background property definition directly under the styles property.

Testing Instructions

Create a JSON file like the one below locally and make sure that the background property is only allowed at the top level.

{
	"$schema": "https://raw.githubusercontent.com/WordPress/gutenberg/fix/theme-json-schema-root-background/schemas/json/theme.json",
	"version": 2,
	"styles": {
	}
}

Screenshots or screencast

4a5a3d161e265e1414cef326e3f031f0.mp4

@t-hamano t-hamano added the [Type] Developer Documentation Documentation for developers label Mar 5, 2024
@t-hamano t-hamano self-assigned this Mar 5, 2024
@t-hamano t-hamano requested review from ramonjd and andrewserong March 5, 2024 13:54
Copy link

github-actions bot commented Mar 5, 2024

Flaky tests detected in 9192442.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8157326424
📝 Reported issues:

Comment on lines -207 to -219
### background

Background styles

| Property | Type | Props |
| --- | --- |--- |
| backgroundImage | string, object | |
| backgroundPosition | string, object | |
| backgroundRepeat | string, object | |
| backgroundSize | string, object | |

---

Copy link
Contributor Author

@t-hamano t-hamano Mar 5, 2024

Choose a reason for hiding this comment

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

Unfortunately, if we update the JSON schema correctly, the background section will be missing from the documentation 😅 This is because the script that generates the documentation relies only on stylesProperties and does not consider which level the properties are available.

We may need to make major changes to the logic of the script, or the overall design of the JSON schema, to ensure that it also generates the documentation correctly.

Personally, I think it's better not to have a background property at this point, because if there is a background property in the document, there is a risk of misunderstanding that this property is available at all levels.

Copy link
Member

Choose a reason for hiding this comment

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

Agree. Thanks for noticing this detail!

@t-hamano t-hamano marked this pull request as ready for review March 5, 2024 14:45
Copy link

github-actions bot commented Mar 5, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: t-hamano <[email protected]>
Co-authored-by: ramonjd <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

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.

The background property is defined as a subproperty of stylesProperties. However, the stylesProperties is also used at the block/element level.

The plan is to allow background a lower levels in the not-too-distant future, but this change makes sense for now and can be reverted later when support is there.

Comment on lines -207 to -219
### background

Background styles

| Property | Type | Props |
| --- | --- |--- |
| backgroundImage | string, object | |
| backgroundPosition | string, object | |
| backgroundRepeat | string, object | |
| backgroundSize | string, object | |

---

Copy link
Member

Choose a reason for hiding this comment

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

Agree. Thanks for noticing this detail!

@t-hamano
Copy link
Contributor Author

t-hamano commented Mar 6, 2024

@ramonjd Thanks for the review!

@t-hamano t-hamano merged commit cabc0c7 into trunk Mar 6, 2024
65 checks passed
@t-hamano t-hamano deleted the fix/theme-json-schema-root-background branch March 6, 2024 01:58
@github-actions github-actions bot added this to the Gutenberg 17.9 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants