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

Add config/settings_schema.json JSON schema #536

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Apr 8, 2024

  • Better markdown for input settings docs
  • Slight refactor of input-settings.json for reuse in theme_settings.json
    • before: input-settings.json
    • now:
      • settings.json
      • setting.json
    • initially because I wanted to split sidebar settings from input settings, but then I realized the enums wouldn't merge in an allOf rule, so I changed my mind and put them back in the same schema.
  • Add Shopify reference docs to every input setting and sidebar setting type
  • Add relevant docs to every input setting and sidebar setting type
  • Add config/settings_schema.json JSON schema

Fixes Shopify/develop-advanced-edits#149

settings-validation.mp4

To tophat:

  • Pull PR
  • Open any tests/fixtures/theme-settings-*.json file, the file association is baked into the VS Code settings of the repo.

@charlespwd charlespwd requested a review from a team April 8, 2024 17:26
Comment on lines +2 to +21
"json.schemas": [
{
"fileMatch": [
"tests/fixtures/section-*.json"
],
"url": "./schemas/theme/section.json"
},
{
"fileMatch": [
"tests/fixtures/translations-*.json"
],
"url": "./schemas/theme/translations.json"
},
{
"fileMatch": [
"tests/fixtures/theme-settings-*.json"
],
"url": "./schemas/theme/theme_settings.json"
}
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how we can have VS Code do the intellisense automatically in the fixtures folder. Makes dev-ing things rather easy :)

Comment on lines +8 to +11
"const": "article",
"description": "A setting of type article outputs an article picker field that's automatically populated with the available articles for the store. You can use these fields to capture an article selection, such as the article to feature on the homepage.",
"markdownDescription": "A setting of type `article` outputs an article picker field that's automatically populated with the available articles for the store. You can use these fields to capture an article selection, such as the article to feature on the homepage.\n\n---\n\n[Shopify reference](https://shopify.dev/docs/themes/architecture/settings/input-settings#article)"
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out that the way to have a "documented enum" is to have a oneOf rule of const. Does the same thing but each const can be documented.

Choose a reason for hiding this comment

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

Makes sense.
Is this something in the official docs or did you do some detective work to find out about this?

I tried to find some explanations on my own and found some info on stackoverflow and github

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(For documentation purposes): Discovered it from trial and error. It's much more verbose, but also probably easier to maintain in the new setup.

schemas/theme/setting.json Outdated Show resolved Hide resolved
schemas/theme/setting.json Outdated Show resolved Hide resolved
tests/test-helpers.ts Outdated Show resolved Hide resolved
Also, add proper validation for color_scheme_group.

It's a little less DRY, but also more declarative and easier to understand.
@charlespwd charlespwd force-pushed the feature/better-input-settings-docs branch from a9fd75e to 3be3533 Compare April 9, 2024 14:55
"default": true,
"label": true,
"info": true,
"id": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need those so that additionalProperties validates correctly.

https://json-schema.org/understanding-json-schema/reference/object#extending

Copy link

@jamesmengo jamesmengo left a comment

Choose a reason for hiding this comment

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

I did not validate every single definition, but I 🎩 with the fixtures and messed around with things

is there value in adding a a translation schema fixture as well?
Happy to take a stab at it if you think there is.

schemas/theme/settings.json Show resolved Hide resolved
@@ -25,8 +25,7 @@
"maximum": 2
},
"settings": {
"description": "Section specific settings.",

Choose a reason for hiding this comment

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

AFAIK, we display the markdownDescription on hover - where would the description provided here render?


Followup -> It seems like it will go with

  1. $ref -> markdownDescription
  2. $ref -> description
  3. description from this file

"properties": {
"type": {
"const": "article",
"description": "A setting of type article outputs an article picker field that's automatically populated with the available articles for the store. You can use these fields to capture an article selection, such as the article to feature on the homepage.",

Choose a reason for hiding this comment

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

Q for learning: do we need both description and markdownDescription?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

markdownDescription is a undocumented feature of vscode-json-languageservice that lets you resolve links and stuff. Didn't want to have markdown links in editors that don't support it, so I used markdownDescription to put in links.

Comment on lines +8 to +11
"const": "article",
"description": "A setting of type article outputs an article picker field that's automatically populated with the available articles for the store. You can use these fields to capture an article selection, such as the article to feature on the homepage.",
"markdownDescription": "A setting of type `article` outputs an article picker field that's automatically populated with the available articles for the store. You can use these fields to capture an article selection, such as the article to feature on the homepage.\n\n---\n\n[Shopify reference](https://shopify.dev/docs/themes/architecture/settings/input-settings#article)"
},

Choose a reason for hiding this comment

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

Makes sense.
Is this something in the official docs or did you do some detective work to find out about this?

I tried to find some explanations on my own and found some info on stackoverflow and github

@charlespwd charlespwd merged commit ee941ec into main Apr 9, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants