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

Fixed documentation inside the template #39

Merged
merged 9 commits into from
Aug 6, 2024

Conversation

santacodes
Copy link
Collaborator

As brought up in this discussion the documentation should build now without any errors.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'll let @agriyakhetarpal (pybamm's goto documentation person, haha) do the final review

Copy link
Member

@brosaplanella brosaplanella 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, just see a few comments

.github/workflows/test_on_push.yml Show resolved Hide resolved
template/docs/_static/pybamm.css Outdated Show resolved Hide resolved
template/noxfile.py.jinja Show resolved Hide resolved
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @santacodes, some comments below

template/docs/_static/pybamm.css Outdated Show resolved Hide resolved
template/docs/_static/pybamm.css Outdated Show resolved Hide resolved
template/docs/_static/pybamm.css Outdated Show resolved Hide resolved
template/docs/conf.py.jinja Show resolved Hide resolved
Copy link
Member

@arjxn-py arjxn-py 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

@santacodes
Copy link
Collaborator Author

@agriyakhetarpal do you have any ideas on what we could replace the pybamm.css file with in the _static folder? I think it would be good to merge after that. Refer here.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Thanks, @santacodes! I think it is better to remove the CSS file altogether and keep html_css_files = ["custom.css"] commented out in conf.py since the PyData Sphinx Theme already provides its own styles. The CSS file contains a lot of PyBaMM-specific configuration that users can copy manually if they wish to adjust and tinker with things. This is the only change I would request – the rest of the code looks good to me, and I'm glad to approve this PR after that.

.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
@santacodes
Copy link
Collaborator Author

santacodes commented Aug 6, 2024

Thanks, @santacodes! I think it is better to remove the CSS file altogether and keep html_css_files = ["custom.css"] commented out in conf.py since the PyData Sphinx Theme already provides its own styles. The CSS file contains a lot of PyBaMM-specific configuration that users can copy manually if they wish to adjust and tinker with things. This is the only change I would request – the rest of the code looks good to me, and I'm glad to approve this PR after that.

The doctests would fail without something inside the _static folder, as we cannot copy an empty directory while generating a project with copier. Is there something else we could add in the _static folder just as a dummy to keep the tests running or is there a way to suppress the warning for that?

In reference to this

@agriyakhetarpal
Copy link
Member

Oh, I see. In that case, we can add an empty .gitkeep file inside the _static folder, which is likely what you need.

Copy link
Member

@agriyakhetarpal agriyakhetarpal 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 the fixes, @santacodes!

@agriyakhetarpal agriyakhetarpal merged commit edd627c into pybamm-team:main Aug 6, 2024
36 checks passed
@santacodes santacodes deleted the docfixes branch August 6, 2024 13:33
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.

5 participants