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

JSON Schema Reorganization and Fixes #63591

Merged
merged 20 commits into from
Jul 23, 2024
Merged

JSON Schema Reorganization and Fixes #63591

merged 20 commits into from
Jul 23, 2024

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Jul 16, 2024

What?

Post-update code organization for JSON schema.

JSON structure changes:

  • Inline useRootPaddingAwareAlignments property.
  • Inline fontFace items.
  • Sort settingsProperties alphabetically.
  • Ensure every subschema with properties has "type": "object".
  • Ensure every subschema with propertyNames has "type": "object".
  • Alphabetize stylesProperties.

Fixes:

  • Remove references to WordPress versions.
  • Add missing ref support for textAlign and textColumns.
  • Reuse #/definitions/stylesElementsPseudoSelectorsProperties

Why?

Follow-up to #62462 to fix some things found while working on it.

How?

See the commits for details.

Testing Instructions

Check that the new docs and new schema look correct.

Do all of the properties seem like they're in an intuitive order that makes sense?

Is the JSON file readable? Would you more easily be able to figure out where to add a new property if you need to?

Testing Instructions for Keyboard

N/A

Screenshots or screencast

N/A

@ajlende ajlende added the [Type] Code Quality Issues or PRs that relate to code quality label Jul 16, 2024
@ajlende ajlende self-assigned this Jul 16, 2024
Copy link

Flaky tests detected in b313e3b.
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/9950837869
📝 Reported issues:

Copy link

github-actions bot commented Jul 17, 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: ajlende <ajlende@git.wordpress.org>
Co-authored-by: scruffian <scruffian@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>

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

@ajlende ajlende requested a review from t-hamano July 17, 2024 03:29
@@ -39,14 +39,14 @@ Setting that enables the following UI tools:

---

### useRootPaddingAwareAlignments
Copy link
Contributor Author

Choose a reason for hiding this comment

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

useRootPaddingAwareAlignments is removed in this PR, but returned in #63639. I wanted to keep the codegen changes in a separate PR, but the existing codegen didn't support the refactored structure which is why it is missing here.

@ajlende ajlende requested review from ntwb and nerrad as code owners July 17, 2024 15:26
@ajlende ajlende changed the title JSON Schema Cleanup JSON Schema Reorganization and Fixes Jul 17, 2024
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

Do all of the properties seem like they're in an intuitive order that makes sense?

Yeah, alphabetising them makes sense to me, and is as good as any other approach I can think of!

Is the JSON file readable? Would you more easily be able to figure out where to add a new property if you need to?

Yeah, it looks clear to me.

Copy link
Contributor

@t-hamano t-hamano 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 the cleanup!

Here are some things I noticed, but they're very minor and may not need any action.

If the description field has highest priority in sorting, would it be better to move these fields as well?

"description": "Array of category slugs.",

"title": {
"type": "string",
"description": "Title of the global styles variation. If not defined, the file name will be used."
},
"slug": {
"type": "string",
"description": "Slug of the global styles variation. If not defined, the kebab-case title will be used."
},
"description": {
"type": "string",
"description": "Description of the global styles variation."
},
"blockTypes": {
"type": "array",
"description": "List of block types that can use the block style variation this theme.json file represents.",
"items": {
"type": "string"
}
},

Do these need type:string?




Is this type:object unnecessary?

"type": "object",

schemas/json/theme.json Outdated Show resolved Hide resolved
schemas/json/theme.json Outdated Show resolved Hide resolved
@scruffian scruffian force-pushed the update/json-schema-draft-07 branch from 7f1a9f1 to 5fce792 Compare July 19, 2024 11:44
@t-hamano t-hamano self-requested a review July 20, 2024 11:09
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

LGTM! To ship this PR, we need to merge #63583 first, right?

@ajlende
Copy link
Contributor Author

ajlende commented Jul 22, 2024

If the description field has highest priority in sorting, would it be better to move these fields as well?

Yeah, looks like I missed some. I did a search through the project for the regex (?<!\{\n\s*)"description" which should turn up any descriptions that are not first in their object, and fixed all of them now.

I also did a similar search for "default": [^,]+, which should be the same for the defaults at the end.

Do these need type:string?

All propertyNames are evaluated as strings according to the specification, so I didn't include it if the enum was for propertyNames. But since it doesn't matter if it's included or not, maybe the consistency of always including a type with an enum would help people remember to add it in the other places too?

Do you have an opinion on that? I can see it either way.

Is this type:object unnecessary?

No, but I should have moved it alongside propertyNames.

One of my goals with the refactoring was to make sure that all instances of "allOf" included complete schemas with types inside and nothing outside. Technically it isn't invalid to have the "type": "object" outside, but I think it'll be more clear that each subschema gets validated independently of the others this way.

@ajlende ajlende force-pushed the update/json-schema-cleanup branch from bf6403a to 221349d Compare July 23, 2024 14:28
@ajlende ajlende force-pushed the update/json-schema-cleanup branch from 221349d to e5ed758 Compare July 23, 2024 15:18
@ajlende ajlende force-pushed the update/json-schema-cleanup branch from e5ed758 to 2546af7 Compare July 23, 2024 15:50
@ajlende ajlende enabled auto-merge (squash) July 23, 2024 16:07
@ajlende ajlende merged commit e39dad5 into trunk Jul 23, 2024
60 checks passed
@ajlende ajlende deleted the update/json-schema-cleanup branch July 23, 2024 16:26
@github-actions github-actions bot added this to the Gutenberg 18.9 milestone Jul 23, 2024
westonruter added a commit that referenced this pull request Jul 23, 2024
* trunk: (2604 commits)
  Update "Versions in WordPress" page (#63869)
  SelectControl: Fix hover/focus color in wp-admin (#63855)
  Add margin-bottom lint rules for RangeControl (#63821)
  JSON Schema Docgen Rework (#63868)
  JSON Schema Reorganization and Fixes (#63591)
  DataForm: Add a simple story for the DataForm component (#63840)
  Quick Edit: Support bulk selection (#63841)
  Update dataviews docs (#63860)
  Bump the github-actions group across 1 directory with 4 updates (#63808)
  Add unit tests for the gutenberg_render_block_core_post_title() function.
  Make hover block outlines not present in Distraction Free (#63819)
  DataViews: Rename the header property of fields to label (#63843)
  Fix: Error while Calling edit-site getCurrentTemplateTemplateParts selector (#63818)
  Revert "Update HeightControl component to label inputs" (#63839)
  Zoom out: hide vertical toolbar when block is not full width (#63650)
  Latest comments: Add color block support (#63419)
  Core Data: Remove leftover 'todo' comment (#63842)
  Tabs: keep full opacity of focus ring on disabled tabs (#63754)
  Fix selected row styles in table layout (#63811)
  Align checkbox, radio, and toggle input design (#63490)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
Development

Successfully merging this pull request may close these issues.

3 participants