-
Notifications
You must be signed in to change notification settings - Fork 132
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
1068 Generating context docs from schema files #1151
1068 Generating context docs from schema files #1151
Conversation
|
✅ Deploy Preview for fdc3 canceled.
|
@kriswest @robmoffat - first iteration is done; please check the description above to see what's been done and what has not; there's also a preview on Netlify pointing to my fork (the build command will need to be updated when we go live with this). Eager to hear your feedback! /cc @mistryvinay |
Hi @julianna-ciq , thanks SO much for the detailed review! @TheJuanAndOnly99 is currently off, he'll be back on March 3rd and work on your comments. |
Co-authored-by: Julianna Langston <74684272+julianna-ciq@users.noreply.github.com>
Co-authored-by: Julianna Langston <74684272+julianna-ciq@users.noreply.github.com>
…examples, refactor context ref split
Hey @julianna-ciq thank you so much, great review, really appreciate your feedback. I've implemented all your notes. Would you mind reviewing again. If everything looks good do you think it's okay to merge? (Please note that after this is merged we would have to update the build command on Netlify). |
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.
I have a couple of stylistic nitpicks, but I think it looks good. I think @kriswest or @robmoffat have to review as well, though.
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.
Could we have a link to the schema file on the page, so that the user can view that?
Also, might be good to reference the Typescript type that is generated, if that's possible (and c# if that's possible too I guess)
Hi @kriswest I fixed the build issue but I don't see it triggering the check again. |
@TheJuanAndOnly99 I've manually re-run it. I know its going to fail (as the docusaurus website has developed an issue with a dependency, but the log will tell us whether anything else is still amis). |
@TheJuanAndOnly99 you're all clear! Just he axios dependency issue in the log |
@mistryvinay and I should be able to take it from here - we need to run through another review and then make the tweaks necessary to have the generated pages replace the existing ones, meaning they'll need to generate onto the same URLs as the manual pages, which we'll delete! |
...although @TheJuanAndOnly99 you might need to explain or handle whatever is needed regarding the netlify preview/build... I don't yet know what change is needed nor why. If its about making sure the generation script gets run, we could possibly do that with a commit hook and npm package script... |
@kriswest when this PR gets merged I need to update the build command on Netlify to run the script and that will take care of things for us. |
@TheJuanAndOnly99 The preview for this appears to be broken - could you take a look, not seeing the generated pages anymore. Perhaps a conflict on updating from @robmoffat's homepage update |
@TheJuanAndOnly99 I'm not seeing the generated pages in the preview's sidebar anymore: These are the old pages?, right: https://exquisite-otter-12d363.netlify.app/docs/next/context/ref/Action |
@kriswest yep you are correct! I think I've fixed it. https://issue-1068--exquisite-otter-12d363.netlify.app/docs/context/schemas/Action |
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 looks fine. I went through the Context pages comparing the most recently provided preview to the current docs, here are my notes.
General differences:
- No Overview page for Context Data Part
- No Context page for Context Data Part
- ‘Required’ boolean field is missing for all properties
- Schema link goes to github.com instead of fdc3.finos.org, assume that will update on publish/deploy
- All reference links are broken in the Netlify preview, is this expected?
- All properties with array types no longer have specific array type (like Instrument[]), they just say ‘array’. Many do have a sentence that says what kind of array it is, and some have reference links to the schema page for the thing it is an array of
- Properties with a reference link to another docs page no longer have ‘type’ explicitly declared, this is probably fine as the information is present in the page linked
- I find the new format a little difficult to read, I think some indentation for the data under each property would go a long way.
Page specific differences:
- Chart
o Instruments property used to have type ‘Instrument[]’ now it has ‘array, is clear from the description that this is an array of instruments - ChatInitSettings
o Message property used to be of type ‘Message’, new one has an example but should probably have a reference link to fdc3.message - Chat Search Criteria
o Properties property used to have type (Instrument | Contact | Organization | string)[] , now it just has type ‘array’ - Contact
o No longer has ‘name’ property - ContactList
o No longer has ‘id’ or ‘name’ properties
o ‘contacts’ property could have a reference link to the contact schema page - Country
o No longer has ‘name’ property - Email
o ‘recipients’ property used to be of type ‘fdc3.contact or fdc3.contactList’, now it has no type and no reference link - Instrument
o ‘id’ subproperties used have working links under ‘More Info’, now they have URLs that aren’t active hyperlinks links displayed like<https://www.isin.org/>
- InstrumentList
o No longer has ‘name’ or ‘id’ properties
o ‘instruments’ property could have a reference link to instrument schema page - Order
o Details.product has type and description ‘undefined’ - OrderList
o No longer has ‘name’ or ‘id’ properties
o ‘orders’ property used to be of type ‘Trade[]’ which I believe is incorrect, I think it should be Order[], now it just has type ‘array’ but does not have a reference link to the order schema page - Organization
o Missing ‘name’ property - Portfolio
o Missing ‘name and ‘id’ properties
o ‘positions’ property could link to position schema page - Position
o Missing ‘name ‘ and ‘id’ properties - TradeList
o Missing ‘id’ and ‘name’ properties
o Trades property could link to the trades schema page - TransactionResult
o Missing ‘message’ property
The missing |
Checklist for changes that need to be made to close this (from @hughtroeger very detailed review) Integration into project
Generated content
Page specific differences
|
@TheJuanAndOnly99 & @finos/fdc3-maintainers I'm going to merge this into branch on the FINOS repo so we can continue the development there. I'm pretty sure we can get this running with the existing netlify config using npm scripts to trigger it instead (which will be better for local development). Many thanks for your help with this! We'd happily take more of course ;-) I'll tag you in the new PR once this is moved over. |
8425b34
into
finos:1068-context-docs-generation
resolves #1068
First iteration is complete:
yarn schema2markdown
in the root directory.Todo:
Preview - https://issue-1068--exquisite-otter-12d363.netlify.app/docs/context/schemas/Action