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

Update documentation to use sphinx #643

Merged
merged 31 commits into from
Dec 11, 2024
Merged

Conversation

ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Dec 6, 2024

Motivation

Previous documentation relied on outdated scripts. This PR adds tools for autogenerating documentation using sphinx-matlabdomain.

New documentation page: https://matnwb.readthedocs.io/en/latest/index.html

Todo

  • Move the Data Dimensions section to a page in the RTD
  • Modify the README page to point users to the RTD.
  • Improve the height of the iframe.
  • The tutorials contain links to the old API docs, and will need to be update to the new API docs
  • Favicon - added, not working in safari
  • Include links to the youtube video walkthrough

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 99.08257% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.23%. Comparing base (4bf41dc) to head (e608c5f).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
+file/fillConstructor.m 98.64% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #643      +/-   ##
==========================================
+ Coverage   95.15%   95.23%   +0.08%     
==========================================
  Files         113      115       +2     
  Lines        4768     4873     +105     
==========================================
+ Hits         4537     4641     +104     
- Misses        231      232       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bendichter
Copy link
Contributor

bendichter commented Dec 9, 2024

This looks really great! The tutorials and API docs both look really nice. I love the cross-links in the API docs to the schema and schema docs for each of the classes.

I think the main remaining point here would be to modify the README.md. We should:

  • Move the Data Dimensions section to a page in the RTD
  • Modify the README page to point users to the RTD.

I have a few very minor suggestions that should not be blocking:

  • Improve the height of the iframe. Currently, the tutorials are displayed in the sphinx page as an iframe. The way it is currently done, the height of the iframe matches the height of the window. This results in two scroll wheels, one for the outer page and one for the iframe. It would be a bit better if the iframe height were increased to match the height of the content, so you would only be left with the outer scroll wheel.
  • It would be great to add a the matnwb logo as a favicon instead of using the default RTD favicon.

Next steps:

  • The tutorials contain links to the old API docs, and will need to be update to the new API docs

@ehennestad
Copy link
Collaborator Author

The tutorials contain links to the old API docs, and will need to be update to the new API docs

Should have done this already. Maybe refresh browser? Let me know if you find links pointing to the old docs

@bendichter
Copy link
Contributor

Ah yes you have done a great job of fixing the inline links within the tutorials. There is also this section at the end of several tutorials:

image

which points to the old links. At this point, honestly we could just remove this section. I think it's pretty clear how to navigate to the other tutorials and the relevant documentation using the current RTD structure.

@bendichter
Copy link
Contributor

Would it be possible to include links to the youtube video walkthroughs of the tutorials within the page?

@ehennestad
Copy link
Collaborator Author

ehennestad commented Dec 9, 2024

which points to the old links. At this point, honestly we could just remove this section. I think it's pretty clear how to navigate to the other tutorials and the relevant documentation using the current RTD structure.

It might be nice to keep these links as convenience for people exploring the livescripts in MATLAB.
I think the best option is to make the links relative (to other tutorials at least). This way, when people follow the link in MATLAB it will open the live script instead of the browser, whereas if people browse it online, it will open the the html page in the browser.

It seems MATLAB's html-export automatically changes the extension of links from .mlx to .html so this should work without interventions as long as the relative locations are kept consistent (I did not test it yet, but will try)

@bendichter
Copy link
Contributor

Looks awesome! Let's merge as soon as the tests pass

@bendichter
Copy link
Contributor

@ehennestad do you want to remove the draft status?

@ehennestad
Copy link
Collaborator Author

I want to add a section about the dimensions and how text in the documentation about time series dimensions are inconsistent with how matnwb works

@ehennestad
Copy link
Collaborator Author

add section from matnwb readme
add pages from nwb-overview
minor rearrangement
@ehennestad ehennestad marked this pull request as ready for review December 9, 2024 15:00
@bendichter bendichter self-requested a review December 9, 2024 15:01
bendichter
bendichter previously approved these changes Dec 9, 2024
@ehennestad ehennestad merged commit 9b73801 into master Dec 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants