-
-
Notifications
You must be signed in to change notification settings - Fork 57
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 a guide on the upcoming data maps #46
Conversation
Deploying with Cloudflare Pages
|
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.
Good for a first pass.
Few comments, but it generally feels difficult to read and understand if you don't already know how the system works. It reads as though there is a lot of assumed knowledge about the system.
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.
Added a few more comments as I'm trying to understand this system better.
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.
One quick comment. I think the rest is fine, though I would like one more member of the doc team to weigh in.
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.
Generally this looks good. My main complaint is that I find it weird to split the JSON structure off into a separate article. I believe it would make more sense to include that in the main article's Usage section.
I'm also not completely sold on keeping the builtin maps in their own article, however if more systems like this are planned, then it's probably best to keep the article separate.
I put the json structure in a different page because the main page is for developers and the json structure affects pack devs and users. |
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 overall, just a few minor points
Co-authored-by: Dennis C <[email protected]>
6c040b9
Co-authored-by: Dennis C <[email protected]>
Co-authored-by: Dennis C <[email protected]>
Co-authored-by: Dennis C <[email protected]>
Depends on neoforged/NeoForge#519 and a PR that makes compostables a data map.
Preview URL: https://pr-46.neoforged-docs-previews.pages.dev