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 change for spacingSizes pattern incorrect #62520

Closed
justintadlock opened this issue Jun 12, 2024 · 4 comments · Fixed by #62567
Closed

theme.json schema change for spacingSizes pattern incorrect #62520

justintadlock opened this issue Jun 12, 2024 · 4 comments · Fixed by #62567
Assignees
Labels
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 [Type] Regression Related to a regression in the latest release

Comments

@justintadlock
Copy link
Contributor

justintadlock commented Jun 12, 2024

With this change to spacingSizes and theme.json v3, the schema was changed to specifically add a numeric string pattern instead of any string for the slug property: #61842

If I use a non-numeric string for slug, this gave no warning in the past:

"settings": {
	"spacing": {
		"spacingSizes": [
			{
				"name": "Base",
				"size": "clamp(1.06rem, 0.37cqi + 0.95rem, 1.19rem)",
				"slug": "base"
			}
		]
	}
}

Now, VS Code or other schema checkers will show a warning that the slug doesn't match the pattern.

I am concerned that theme authors who were previously using non-numeric strings in the past will think they must migrate/upgrade their spacing sizes in some way, which would not be something we'd want to ask theme authors to do.

Not to mention that the Theme Handbook has shown non-numeric slugs as valid examples since last year: https://developer.wordpress.org/themes/global-settings-and-styles/settings/spacing/

I think the new description is fine:

Unique identifier for the space size preset. Will be sorted numerically. For best cross theme compatibility these should be in the form '10','20','30','40','50','60', etc. with '50' representing the 'Medium' size step.

However, I would remove the pattern from the schema because non-numeric strings can and still do work in v3.

CC: @ajlende

@justintadlock justintadlock added [Type] Regression Related to a regression in the latest release Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jun 12, 2024
@justintadlock
Copy link
Contributor Author

For reference, here's a social conversation where some of this confusion is already arising: https://x.com/mikemcalister/status/1800919555063124106 (specific part of the thread that led me to opening this ticket)

@ajlende
Copy link
Contributor

ajlende commented Jun 12, 2024

I'm a bit torn on this one. The theme.json schema is intended to warn users of problems that they may have with their configuration, and using non-numeric slugs may cause issues with presentation. This will become more visible when user-defined presets can be added via the global styles UI as the user ones will be merged with the theme ones even if default presets are disabled.

The flagged issues in a theme.json by the schema are just warnings—they don't affect how global styles functions, but it seems that some people are treating them as more than that. Changing a slug would be a breaking change for a theme since all existing posts and pages use the old slugs, and we don't want people breaking their themes to fix an underline in their editor.

At the same time, you want folks with new themes to use numeric slugs for the "correct" experience. But I don't think most folks read the description unless they don't think they understand what the option does. The convention for other presets—including fontSizes which also could have been ordered—is not to use numeric slugs. I probably wouldn't read the description either to find out that they should be numeric.

The original documentation for slugs didn't strictly require numeric slugs, but still suggested them "for best cross theme compatibility". And, like you shared, our own documentation isn't following the convention that we set. The description just isn't enough to convince users to use numeric slugs.

It would be nice if editors had a way to disable some of the warnings that you don't care about like this one for existing themes. One possible way around it is adding a "strict mode" schema for theme.json. Unfortunately, that entails manually maintaining two different JSON schemas for the same thing or writing some code to try to keep them in sync somehow.

Or maybe we could add a heuristic to sort based on the size. Since we're running this in PHP, you can't just getComputedStyle have a proper sorting, but you could probably write something to evaluate the calculation based on the CSS spec as long as it isn't using CSS custom properties.

Maybe a better solution would be a UI one where different origins aren't merged and sorted at all. That's what I had initially for the spacingSizes PR, but it was brought up as being confusing when they all become one list in the select UI and weren't in increasing order. If we had a Select component that could be customized to having headers for sections, then maybe that would make the most sense.

Sorry for rambling. I know that having the requirement in the schema isn't a good experience, but I also don't think it would be a good experience to simply remove the constraint in the schema.

@justintadlock
Copy link
Contributor Author

You've covered a lot of ground, which I think is worth discussing. However, I want to make sure that we're focusing on the specific issue at hand: the change to the schema is already causing developer experience issues during the WP 6.6 beta.

This needs to be addressed in some way before 6.6 ships. When we have some of the community's top theme authors discussing migrations for their slugs, that is very problematic. Even I was a bit confused and needed to dig to find out what was going on. It wasn't clear from the schema whether my code would work.

In VS Code, the warning from the schema tells developers that the String does not match the pattern of "^[0-9].*$". That is not consistent with what has been historically and is currently supported.

The addition of the pattern match was also neither discussed in the PR (#61842) nor mentioned in the Dev Note copy (#58409 (comment)).

Side note: I'd also like to mention that one of my long-term concerns is that, with the pattern added to the schema, future contributors will assume that only numeric strings are allowed, opening the door to other potential bugs in Gutenberg/Core. We've already seen that assumption play out in this cycle (which, great job addressing): #62199

I do think there are some good longer-term discussions to have around your concerns. However, those are less immediate than the current issue for WP 6.6 (and, as far as I know, a change for 6.6 wouldn't negatively affect changes we could implement in the future).

I still believe, at least for now, removing the regex pattern is the best path to avoid DX issues in 6.6.

Alternatively, perhaps we could flesh out the description to say that any alphanumeric strings with hyphens or underscores are valid. This would at least give context to developers who are suddenly seeing warnings in their editors that did not exist before. I do have my concerns about that if we want to start recommending a convention, though.

@ajlende
Copy link
Contributor

ajlende commented Jun 14, 2024

We have plenty of time before 6.6 ships to change the schema. The schemas are served directly from GitHub, so backporting it only involves adding the commit to the wp/6.6 branch.

I'd prefer to have a nicer solution for 6.6 than just the schema change. And I have some capacity to work on it right now.

After giving my thoughts some more time to develop, I think I have a decent solution.

The heuristics for sorting that I added in #62199 were focused on figuring out v2 vs v3 schemas. If we do away with heuristics and simply sort when all slugs start with a number, the sorting becomes an opt-in feature by the theme using numeric slugs. The numeric slugs become a bonus feature rather than a requirement for correct operation.

We'll still want to update the UI to have sections, but I don't think that's critical to fix until we have UI for adding user origin spacing presets. At that point, if a user adds a new preset that isn't numeric, then that will effectively opt-out of the merging and sorting of presets even if the theme is using numeric slugs.

Does that sound okay? I opened #62567 if you want to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 [Type] Regression Related to a regression in the latest release
2 participants