-
Notifications
You must be signed in to change notification settings - Fork 841
Fix the invalid standard_catalog.json #614
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
Conversation
google#605 accidentally broke the v0.9/standard_catalog.json. This commit fixes the invalid JSON schema and reformats it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes an invalid JSON schema in standard_catalog.json by correcting the structure and reformatting the file. The changes look good and make the schema valid.
I've added a few suggestions to further improve the schema's correctness and maintainability:
- In
ChoicePicker, the type of thevalueproperty should depend on thevariantto correctly handle single vs. multiple selections. - The
minandmaxproperties of theSlidercomponent could be made dynamic to allow for more flexible UIs. - The duplicated definitions for
minandmaxinDateTimeInputcould be refactored into a reusable type in$defsto improve maintainability.
| "min": { | ||
| "type": "number", | ||
| "description": "The minimum value of the slider." | ||
| }, | ||
| "max": { | ||
| "type": "number", | ||
| "description": "The maximum value of the slider." | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The min and max properties of the Slider component are currently defined as static numbers. To allow for more flexible UIs where the slider's range can be determined at runtime, consider changing them to DynamicNumber, similar to the value property. This would allow binding the range to the data model.
"min": {
"$ref": "common_types.json#/$defs/DynamicNumber",
"description": "The minimum value of the slider."
},
"max": {
"$ref": "common_types.json#/$defs/DynamicNumber",
"description": "The maximum value of the slider."
}| "min": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "common_types.json#/$defs/DynamicString" | ||
| }, | ||
| "then": { | ||
| "oneOf": [ | ||
| { | ||
| "format": "date" | ||
| }, | ||
| { | ||
| "format": "time" | ||
| }, | ||
| { | ||
| "format": "date-time" | ||
| } | ||
| ] | ||
| { | ||
| "if": { | ||
| "type": "string" | ||
| }, | ||
| "then": { | ||
| "oneOf": [ | ||
| { | ||
| "format": "date" | ||
| }, | ||
| { | ||
| "format": "time" | ||
| }, | ||
| { | ||
| "format": "date-time" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| ], | ||
| "description": "The minimum allowed date/time in ISO 8601 format." | ||
| }, | ||
| "max": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "common_types.json#/$defs/DynamicString" | ||
| }, | ||
| { | ||
| "if": { | ||
| "type": "string" | ||
| ], | ||
| "description": "The minimum allowed date/time in ISO 8601 format." | ||
| }, | ||
| "max": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "common_types.json#/$defs/DynamicString" | ||
| }, | ||
| "then": { | ||
| "oneOf": [ | ||
| { | ||
| "format": "date" | ||
| }, | ||
| { | ||
| "format": "time" | ||
| }, | ||
| { | ||
| "format": "date-time" | ||
| } | ||
| ] | ||
| { | ||
| "if": { | ||
| "type": "string" | ||
| }, | ||
| "then": { | ||
| "oneOf": [ | ||
| { | ||
| "format": "date" | ||
| }, | ||
| { | ||
| "format": "time" | ||
| }, | ||
| { | ||
| "format": "date-time" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| } | ||
| ], | ||
| "description": "The maximum allowed date/time in ISO 8601 format." | ||
| ], | ||
| "description": "The maximum allowed date/time in ISO 8601 format." | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definitions for min and max in DateTimeInput are identical. To improve maintainability and reduce duplication, you could define a new reusable type DynamicDateTimeString in the $defs section of this file and reference it for both min and max.
Here's how you could define it in $defs:
"DynamicDateTimeString": {
"allOf": [
{
"$ref": "common_types.json#/$defs/DynamicString"
},
{
"if": {
"type": "string"
},
"then": {
"oneOf": [
{
"format": "date"
},
{
"format": "time"
},
{
"format": "date-time"
}
]
}
}
]
}And then use it in DateTimeInput:
"min": {
"$ref": "#/$defs/DynamicDateTimeString",
"description": "The minimum allowed date/time in ISO 8601 format."
},
"max": {
"$ref": "#/$defs/DynamicDateTimeString",
"description": "The maximum allowed date/time in ISO 8601 format."
}|
Closing because this was fixed in #607 |
#605 accidentally broke the v0.9/standard_catalog.json.
This commit fixes the invalid JSON schema and reformats it.
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.