-
Notifications
You must be signed in to change notification settings - Fork 200
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
[ENH, DOC] Allow setting the navbar_max_depth
and collapse_navbar
#605
[ENH, DOC] Allow setting the navbar_max_depth
and collapse_navbar
#605
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
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.
This looks good to me - sorry for missing it before, I think it's OK to avoid testing since we are just inheriting the feature from the pydata theme, as you mention!
@gilbertbw I tried pushing to your branch to fix the merge conflict but got access denied - can you run a merge commit from |
another ping... |
Sorry, I missed the notifications about this. I have merged in master as requested. |
Hi, @choldgraf can I help somehow getting this merged? We'd like to use this option for the ROCm documentation (ROCm/rocm-docs-core#413). |
I rebased the branch to current master, its available here: https://github.com/StreamHPC/sphinx-book-theme/tree/gbw-additional-theme-customisation. Should I make a PR? |
I'll close and re-open this one, let's see how the tests look |
The change and the tests seem OK, though might fail since we haven't rebased on master. Since this one has been waiting a bit I'm just gonna merge and we can see how it impacts the tests and do a spot-fix if necessary. That's easier than opening a whole new PR |
I was not able to figure out how to actually build the docs I wrote for this. I have manually tested that the two new settings work for me.
As this is just exposing functionality from
pydata-sphinx-theme
, I am not sure it needs tests written?fixes #603