-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
Modify non_json_data.md, document contentSchema keyword #1028
Modify non_json_data.md, document contentSchema keyword #1028
Conversation
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 373 373
Branches 94 94
=========================================
Hits 373 373 ☔ View full report in Codecov by Sentry. |
This process ensures that the non-JSON content is properly encoded for transmission and accurately decoded by the recipient, maintaining the integrity of the data throughout the processe. | ||
|
||
<Keywords label="single: contentSchema single: media; contentSchema" /> | ||
|
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.
Based on the content of the contributing.md file and the Google style guide, I has just assumed that we are replacing the "keywords", "star" tags in the new documentation. Now, I'm not so sure about my assumption :). Please let me know. Thank you
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 goal of the review and editing work is to make sure that the documentation uses the tags according to the rules of the style guide: https://github.com/json-schema-org/website/blob/main/pages/md-style-guide.md
That could mean replacing the "Star" tag when it doesn't follow the guidelines with another tag best suited for the context.
Regarding the "Keywords" tag, we don't have it documented in the style guide, yet. But that is a topic for another conversation that I posted on the general channel.
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.
Awesome work! I have left my comments and approved to merge PR after the changes have been implemented.
I asked someone from the TSC to help us with the review. |
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.
Great work. Can we send a message to the TSC channel and Contribute channel asking for feedback for the diagram to make sure is all right?
I just left one minor suggestion.
|
||
To better understand how `contentEncoding` and `contentMediaType` are applied in practice, let's consider the process of transmitting non-JSON data: | ||
|
||
 |
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 image is now being shown. We need to add the image folder to make that work.
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.
Does the website support Mermaid diagrams (Example: https://github.com/json-schema-org/json-schema-spec/pull/1544/files)? If so, it might be nicer because it can be styled to match the rest of the site.
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.
Thanks for the recommendation. I added a version of the diagram in Mermaid to test if it works on our website. I'll remove the PNG file after we confirm it renders correctly.
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.
Looks like the website doesn't support mermaid 😢. That's too bad. Thanks for trying.
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.
Looking good so far. I made a couple suggestions.
|
||
To better understand how `contentEncoding` and `contentMediaType` are applied in practice, let's consider the process of transmitting non-JSON data: | ||
|
||
 |
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.
Does the website support Mermaid diagrams (Example: https://github.com/json-schema-org/json-schema-spec/pull/1544/files)? If so, it might be nicer because it can be styled to match the rest of the site.
Co-authored-by: Jason Desrosiers <[email protected]>
This version includes changes suggested by Jason, Benjamin, Blessing, and Anitha
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.
Everything looks correct from a spec perspective.
These changes introduce a better heading structure for scannability
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.
Looks good to me.
What kind of change does this PR introduce?
Updates non_json_data.md by improving text for readability, adding a figure to illustrate the role of contentEncoding and contentMediaType in sharing non-JSON data, and documenting the contentSchema keyword.
Issue Number:
Screenshots/videos:
If relevant, did you update the documentation?
Yes, this is an update to the following document: https://json-schema.org/understanding-json-schema/reference/non_json_data
Summary
The contentSchema keyword was missing in the document Media: string-encoding non-JSON data. This pull request adds the missing keyword to the document, makes minor improvements to the text, and adds an explanatory image about the role of contentEncoding and contentMediaType in the transmission of non-JSON data.
Does this PR introduce a breaking change? No