-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Comments
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) |
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 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 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 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. |
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 The addition of the Side note: I'd also like to mention that one of my long-term concerns is that, with the 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 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. |
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 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. |
With this change to
spacingSizes
andtheme.json
v3, the schema was changed to specifically add a numeric string pattern instead of any string for theslug
property: #61842If I use a non-numeric string for
slug
, this gave no warning in the past: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:However, I would remove the
pattern
from the schema because non-numeric strings can and still do work in v3.CC: @ajlende
The text was updated successfully, but these errors were encountered: