Test: Validate block & theme json #57374
Conversation
|
Size Change: +14 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
This is also a failure: Line 16 in 87ec413 According to the schema, this is not a valid property. It looks @youknowriad added it in #48893. I'll add it to the schema declaration (please confirm that's correct). |
| @@ -1,6 +1,7 @@ | |||
| { | |||
| "$schema": "https://schemas.wp.org/trunk/block.json", | |||
| "name": "core/widget-area", | |||
| "title": "Widget Area", | |||
There was a problem hiding this comment.
| @@ -2,6 +2,7 @@ | |||
| "$schema": "https://schemas.wp.org/trunk/block.json", | |||
| "apiVersion": 3, | |||
| "name": "core/widget-group", | |||
| "title": "Widget Group", | |||
There was a problem hiding this comment.
|
Flaky tests detected in 1e13fd3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7329432405
|
|
There's a failure in lib/theme.json that was introduced in #42012: Lines 299 to 301 in db5bbf2 Padding is supposed to be an object with top/right/bottom/left properties: gutenberg/schemas/json/theme.json Lines 1492 to 1542 in db5bbf2 I'm assuming a single string works based on #42012 and perhaps should be added to the schema. It looks like the shorthand version of the CSS property. @scruffian can you confirm |
|
I think this is a mistake. It might work, but I think padding should always be an object, not a string. Pinging @tellthemachines @ramonjd @andrewserong for a double check. |
Good question. My understanding is that the global styles code allows handling the single string mostly so that things don't break for folks building out a So my weakly held opinion is that it's good if the code can try not to break if someone uses a shorthand property, but it might be better for the schema to reflect an object for now. If we're considering updating the schema to allow the shorthand property, we'd probably want to thoroughly test that every level of the UI works properly with it first. |
Support for shorthand is currently limited, see #40132. Full support will add a lot of complexity as there are at least three possible formats for it. I'm in favour of not supporting shorthand at all; the schema is there precisely to help folks building theme.json files to know what they can and can't do. The problem is that at this point there are sure to be plenty of themes out there using shorthand already, so if we explicitly stop supporting it we'll break back compat for them. |
Oh, thank you for digging up that issue, I thought we'd talked about it previously! Yes, given we don't support multi-value shorthand, that's a strong argument for keeping shorthand values as invalid in the schema 👍 |
|
Could we modify the shorthand string we're currently using to be valid according to the schema? There's benefit to validating our own files and that's preventing validation from passing right now. |
|
@tellthemachines @andrewserong Is this something one of you can address? My understanding goes as far as "this isn't valid according to the schema" - I don't know much else about how it should work but I would like to get this addressed. |
|
To move this forward, and assuming we're not going to update the theme.json schema to support strings for diff --git a/lib/theme.json b/lib/theme.json
index c2ed7fdca3..2a7b894566 100644
--- a/lib/theme.json
+++ b/lib/theme.json
@@ -297,7 +297,12 @@
"background": "#32373c"
},
"spacing": {
- "padding": "calc(0.667em + 2px) calc(1.333em + 2px)"
+ "padding": {
+ "top": "calc(0.667em + 2px)",
+ "right": "calc(1.333em + 2px)",
+ "bottom": "calc(0.667em + 2px)",
+ "left": "calc(1.333em + 2px)"
+ }
},
"typography": {
"fontSize": "inherit",If folks are okay with that I can push the change - it passes locally for me. |
Thanks @ramonjd, something like that sounds good to me! We'd then just want to manually check that the styling output on themes that don't provide their own button padding looks the same as on The one thing is that in your diff, it looks like the shorthand properties are being copied over, which I don't think is correct. When splitting out to each of the sides, we'd want to grab the appropriate side from the shorthand syntax. So I think |
Oh yeah, I fixed that locally, but didn't update the comment. 😮💨 Thanks for the shoutout. Anyway, I can push and we can always revert. 🤷🏻 |
|
Nice one. Thank you! |
ramonjd
force-pushed
the
fix/test-more-schema-validation
branch
from
a20a9df
to
01e625d
Compare
schemas/json/theme.json
Outdated
| @@ -119,6 +119,11 @@ | |||
| "type": "boolean", | |||
| "default": true | |||
| }, | |||
| "caption": { | |||
There was a problem hiding this comment.
Already fixed here I think: #60017
There was a problem hiding this comment.
Was auto-merged during rebase so I removed the dupe
ramonjd
force-pushed
the
fix/test-more-schema-validation
branch
from
45eb6b6
to
da1ed9e
Compare
ramonjd
requested review from
spacedmonkey,
draganescu,
talldan,
adamziel and
kevin940726
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
force-pushed
the
fix/test-more-schema-validation
branch
from
da1ed9e
to
6ccbc9a
Compare
There was a problem hiding this comment.
Thanks for fixing up the padding rules @ramonjd 🙇 and for the nudges @sirreal!
The update to the padding rules is indistinguishable from trunk in testing for me. It works nicely in block themes, and in Classic themes they wind up getting the fallback button styles from classic.scss, anyway.
The integration test changes to expand testing block.json and add theme.json schema validation all read well for me, too.
LGTM! ✨
| const jsonFiles = glob.sync( | ||
| [ 'packages/*/src/**/block.json', '{lib,phpunit,test}/**/block.json' ], | ||
| { onlyFiles: true } | ||
| ); |
There was a problem hiding this comment.
Just making sure I'm following along: we're expanding the search to block.json files anywhere within packages or tests, so that we pick up the block.json files in the widgets and edit-widgets packages, and anywhere else we might add them in the future, so we don't need to remember to explicitly add those directories? Sounds good to me 👍
| const jsonFiles = glob.sync( | ||
| [ 'packages/*/src/**/theme.json', '{lib,phpunit,test}/**/theme.json' ], | ||
| { onlyFiles: true } | ||
| ); |
There was a problem hiding this comment.
Similarly, we're now saying, any theme.json file anywhere within any of the packages should conform to the https://schemas.wp.org/trunk/theme.json schema. That also sounds good to me 👍
This is just thinking out loud, but I suppose there could theoretically be a use case for referring to a different schema, i.e. a relative path, or something? That seems pretty edge-casey to me, though, and if someone is attempting to do that, then hopefully the schema validation error here will signal to them what's going on, so I don't that's a blocker here.
Remove dupe caption entry
This reverts commit f166d3c.
sirreal
force-pushed
the
fix/test-more-schema-validation
branch
from
6ccbc9a
to
e97a911
Compare
|
Thanks for the help on this folks, I'll land it now 🙌 |
Validate block.json and theme.json files. Fix a few existing minor schema violations. Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: scruffian <scruffian@git.wordpress.org> Co-authored-by: tellthemachines <isabel_brison@git.wordpress.org>
What?
Validate more block.json and theme.json files.
Follow up to #57201 that discovered some invalid block and theme json files.
Why?
These JSON files should be valid according to schemas. If they're invalid, either the schema or the JSON is wrong and should be fixed.
How?
Add and expand integration tests.
Testing Instructions
CI passes. Run these tests locally with
npm run test:unit schema.test.js.You can test with this patch to confirm globs are working as expected: