-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Restore missing handbook pages after #12411 #12452
Conversation
…ting manifest.json
(Also @dd32 how much bandwidth do you have if I have some additional improvements here? Don’t want to overwhelm you with review comments because at this point anything is an improvement and I wouldn’t want this to linger too long!) |
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.
Approving because this is all great, and nothing that explicitly needs to be changed here.
I did add a review note about a new "wildcard" option. That can be addressed here or in a follow-up PR, but I think that's a crucial final step to getting our docs generator in order.
{"docs/designers-developers/developers/data/README.md": "{{data}}"}, | ||
{"docs/designers-developers/developers/packages.md": "{{packages}}"}, | ||
{"packages/components/README.md": "{{components}}"}, |
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.
So one idea here with the packages and data docs and components docs is supporting some kind of wildcard here, e.g.:
{"packages/components/README.md": [
{"packages/components/*": []}
]},
This would allow us to separate the build and the manifest generation, such that there's a distinct build step that generates or syncs the markdown files into the docs/
folder, and then a separate step for adding them to the manifest.json
.
(It's a wildcard rather than explicitly listing files here to allow us to have sections of documentation where the files inside the nesting level may change but the documentation's order doesn't matter. Just a way to simplify a bit here.)
Does that make sense as a way to improve here?
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.
Coming back to this, the components and packages documentation offer some weird pieces here because their documentation lives outside the docs folder and can be quite structurally different.
I guess you'd need to either support full globbing, e.g.
{"packages/components/README.md": [
{"packages/components/**/*": []}
]},
OR… we would have an explicit build step that copies the components/packages documentation into the docs folder first, then a simple wildcard could be used that references the documentation as it exists inside docs, rather than referencing it from elsewhere in the repo.
Thinking out loud, at this point :)
I don't have a huge amount of bandwidth for it right now, I also doubt I have the JS experience required to make such changes honestly. Happy to review or test things though. |
@dd32 totally fair! In that case I’d say let’s merge this and we can iterate later 🙌 |
Description
Following on from #12411 and #11817 it was noticed that some pages were skipped, as they weren't included in the
toc.json
file previously.This PR includes several specific fixes which are all interconnected:
Components
was added to the developers handbook alongsidePackages
andData Package Reference
pages.Packages
,Data Package Reference
andComponents
were removed fromdocs/tool/manifest.js
to prevent them adding as top-level-items. (Are these intended to be top-level-handbook-pages or deep within the developers handbook as the TOC seems to suggest?)I also took the opportunity to convert
generate.php
from the previous PR into a JS command that allows fordocs/root-manifest.json
to be removed entirely and dynamically generated fromdocs/toc.json
so it only needs updating in a single location. That's thegetRootManifest()
andgenerateRootManifestFromTOCItems()
functions which are rather messy, but work (I'm sure someone else could clean that up significantly).How has this been tested?
The only testing done so far is the manual review of the final
docs/manifest.json
.