Skip to content
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 docfx and documentation style guide #103

Merged
merged 35 commits into from
Dec 3, 2024

Conversation

banchan86
Copy link
Contributor

@banchan86 banchan86 commented Sep 19, 2024

Followup to #82, resolves #102.
Accidentally closed the original PR as I was shifting things around but have reproduced the comments below.
This PR introduces two new articles a "Documentation with docfx" and "Documentation style guide"

Review breakdown:

  • "We might want to consider mentioning here how to build the solution in order to export the example workflows, and in that case also consider how to automate the msbuild part into the powershell module containing the export image functions."

    • I have included sections on creating a local environment, setting up build.ps1 in the documentation docfx article, and added more detail to the Bonsai workflow section of the documentation style guide. Its a little over the place, and I think I placed things where they would logically belong, but happy to revise. Im not sure I will be able to carry out 2nd part of that comment.
  • "We should maybe include these as part of docfx-tools or some other template repo."

    • I initially raised a PR to include them as part of the docfx-tools submodule but i realized that maybe wasn't the best solution as users would still need to modify and move them. I am more in favor of hosting them on the docfx-assets repository for users to download but not import as a submodule and have raised a PR to add them. I have also updated the documentation articles with that strategy in mind.
  • "We might want to consider marking the Tutorials section as optional for other package documentation, to make things simpler as a common reference."

    • I have marked this more explicitly as being optional in the top section of the documentation style guide
  • "Review format of relative / local URLs to make sure we have a solution that works for both local and remote builds."

    • So I went to research this and from what I can tell its safer to use ../ rather than ~/ and given that our scripts/code use ../ for navigation, I thought it would be best to standardize on using ./ and ../. I have tested this on docfx articles and it works and I have changed instances of ~/ as well as backward slashes in the documentation articles to reduce ambiguity.
  • "It might be worth mentioning as a note the importance of matching the publishing URL with the PackageProjectUrl attribute specified in the .csproj files. This is where the Bonsai editor will look for the xrefmap.yml file to retrieve URLs for operators."

    • Have added a note to the publishing to github pages section.

Other things that I have changed or moved around since the last time:

  • Decided to use directory trees to better represent file locations and simplify the text in those sections
  • Updated the section that deals with the local bonsai environment given that I now have a better understanding of how that works.
  • Restructured the content in a way I think makes more sense (that lead to the creation of the repository organization, doc organization and version control cleanup sections)
  • Expanded the text so that it reads less like a README and more like an article with proper explaination for why we do certain things, but I may have taken it too far and made it too wordy :)

Online preview of this PR and PRs #97 and #94 : https://banchan86.github.io/bonsai_docs_test/articles/documentation-docfx.html

- and minor edit to README clarity
@banchan86 banchan86 changed the title Newdoc guide Add documentation with docfx and documentation style guide Sep 19, 2024
@banchan86 banchan86 changed the title Add documentation with docfx and documentation style guide Add docfx and documentation style guide Sep 19, 2024
@glopesdev glopesdev self-requested a review November 8, 2024 16:24
@glopesdev glopesdev added the documentation Improvements or additions to documentation label Nov 8, 2024
Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @banchan86, it's a really great start. I did a full first pass over the two guides, this is definitely something we want to have merged soon.

The only thing I am still struggling with is how to move with general infrastructure decisions such as docfx-assets, the centralization or decentralization of package documentation, package documentation index and discoverability, etc.

I will try to make some definite progress on this so we can potentially review this soon, but in the meantime I'm leaving some general feedback that can be worked on independently.

If we can't resolve some of these fundamental questions soon we may want to merge this anyway and iterate later, since this is clearly much better than what we have now (which is either nothing or obsolete content!).

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
@banchan86
Copy link
Contributor Author

banchan86 commented Nov 9, 2024

Thanks for putting this together @banchan86, it's a really great start. I did a full first pass over the two guides, this is definitely something we want to have merged soon.

The only thing I am still struggling with is how to move with general infrastructure decisions such as docfx-assets, the centralization or decentralization of package documentation, package documentation index and discoverability, etc.

I will try to make some definite progress on this so we can potentially review this soon, but in the meantime I'm leaving some general feedback that can be worked on independently.

If we can't resolve some of these fundamental questions soon we may want to merge this anyway and iterate later, since this is clearly much better than what we have now (which is either nothing or obsolete content!).

Sound good, thanks for reviewing it! Accepted almost all the edits, and left comments on a few of the issue to be discussed.

Copy link
Member

@glopesdev glopesdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@banchan86 thanks, all looks good to me, I just noticed a few opportunities to use relative instead of absolute links and after that it should be good to go.

Other than the files in this PR, the only other absolute link I noticed in a cursory look through the entire docs is Line 181 of closed-loop.md where [property mapping operators](http://bonsai-rx.org/docs/property-mapping){:target="\_blank"} could be replaced with [property mapping operators](../articles/property-mapping.md)..

We can also do that one on a separate PR but since it is just the one off maybe there is no harm in adding it here.

articles/documentation-docfx.md Outdated Show resolved Hide resolved
articles/documentation-docfx.md Outdated Show resolved Hide resolved
articles/documentation-docfx.md Outdated Show resolved Hide resolved
articles/create-package.md Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
@glopesdev
Copy link
Member

glopesdev commented Nov 15, 2024

@banchan86 Thanks for the edits, almost there, I have added a final round of suggestions throughout in a final commit directly to the PR branch (4457090), let me know if these look good to you. There are a few things which might still change in the docs short-term but I think I'm happy to push this version and then iterate as we improve.

@banchan86
Copy link
Contributor Author

Looks good to me! Before pushing it though, have we decided what to do with the documentation assets? As the articles point to some files that don't currently exist yet in the docfx-assets repo unless bonsai-rx/docfx-assets#2 is merged in as well.

@glopesdev
Copy link
Member

glopesdev commented Nov 27, 2024

@banchan86 good point about the assets. What I decided to do is actually embed them directly in the guide as code sections, similar to the bits about configuring docfx.json. Turns out most of them are really small and easy to understand, except for the GitHub Actions workflow.

For that one it actually gets even more complicated since we don't have yet a solid standard of how automation should be made, so I deferred with a link to an example workflow for now. We can review later when we have something more refined and stable.

If this sounds good I would be happy merging this.

Copy link
Contributor Author

@banchan86 banchan86 left a 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 overall, and I agree with the approach of just embedding the code snippets for most of the files, but not sure about pointing to the main docs GitHub actions recipe.

articles/documentation-docfx.md Outdated Show resolved Hide resolved
articles/documentation-docfx.md Show resolved Hide resolved
articles/documentation-docfx.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
articles/documentation-style-guide.md Outdated Show resolved Hide resolved
@glopesdev glopesdev self-requested a review December 3, 2024 22:00
@glopesdev glopesdev merged commit 13c5426 into bonsai-rx:main Dec 3, 2024
1 check passed
@banchan86 banchan86 deleted the newdoc-guide branch December 20, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create articles on docfx and documentation style guide
2 participants