-
Notifications
You must be signed in to change notification settings - Fork 29
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
New sidebar structure #138
Conversation
* resolved merge conflicts * Made helpful buttons configurable * Added segment analytics and restyled thankyou msg
…into new-sidebar
…into new-sidebar
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 will need a bit more time to review it thoroughly. The expandable sidebar is great and I also like the README instructions. The translations pages may need special care and we should add needed code to test that in the sample docs. The qiskit meta doc has this file to add the translations_list
and translation_url
.
docs/conf.py
Outdated
'repo_name': 'Example Repo', | ||
'analytics_enabled': True, | ||
'version_list': [0.1, 0.2, 0.3], | ||
'sidebar_headings': [ |
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.
Do you think this is a better approach than adding an additional toctree
in indez.rst
beyond the main one like how qiskit meta is doing?
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.
the only way i could get the dropdowns to work with subpages is by specifying them here. If you just put it in the toctree in index.rst you don't get to customise what goes in the dropdown menu, it will just automatically populate it with the headings on the page and doesn't include sub pages
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.
alternatively we could do something like the qiskit runtime docs with headings and full list of subpages, but then we don't get an expandable dropdown
yes i think some test code in the sample docs would be great! Should that go in your PR or should I try include it here? |
I think it would be better to include here so that we can test before merging this PR. |
I copied over the languages functionality from |
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.
Overall LGTM. I left a few small comments. This is a major improvement for the theme. I am excited to see it get deployed to the docs sites!
README.md
Outdated
|
||
The above toctree will render a sidebar that looks like the image below: | ||
|
||
<img width="348" alt="Screenshot 2022-11-30 at 4 36 26 PM" src="https://user-images.githubusercontent.com/23662430/204913247-761b8202-1bb2-4233-9451-f180b00b5a12.png"> |
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.
The expendable sidebar feature is not shown in the screenshots, probably because the index.rst
files are empty in the example. We should probably clean up the sample doc and make it more like a template and put in things placeholder content under each pages. For this PR though, can you do something like that locally and update these screenshots to show the expandable feature?
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.
That screenshot is not meant to show any expandable items. By default there are no expandable headings unless they explicitly configure them in their setup.py
. This screenshot is meant to show what is displayed when there is just a toctree defined and the screenshots later down show the same sidebar with the additional expandable headings once they're configured :)
qiskit_sphinx_theme/sidebar.html
Outdated
{% if version_list %} | ||
<div class="sidebar-l1-expandable">Previous Releases</div> | ||
<div class="sidebar-subheadings"> | ||
{% for version in version_list | sort(reverse=True) %} | ||
<div class="sidebar-l2"><a href="/documentation/stable/{{ version }}/index.html">{{ version }}</a></div> | ||
{% endfor %} | ||
</div> | ||
{% endif %} |
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.
Just nitpicking. Can you fix the indentation here?
docs/conf.py
Outdated
'subheadings': [ | ||
{ | ||
'title': 'Custom Subtitle 1 (external link)', | ||
'url': 'https://google.com', # use external url |
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.
Can you change the external link to something else? Like https://qiskit.org/ or youtube.com/qiskit
Update: after discussion with @HuangJunye I'm going to refactor this PR to move the configuration aspect out of the This is similar to what Pytorch does: |
Update: I've refactored this so that its now easier to implement the expandable sidebar using just the toctree in the
I think this solution provides the best outcome visually with minimal changes needed to be made by the dev teams themselves. See below examples of the same sidebar with and without expandable sidebars enabled: |
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.
Overall it looks great. Thanks for working on this. I only have a few small comments for README.
README.md
Outdated
2. Refactor the `toctree` in your `index.rst` to separate your pages into different sections. Sections that include a `:caption:` directive will be turned into expandable sidebar sections, with the caption forming the title of the dropdown. for example, to render the expandable sidebar shown above your `toctree` in your `index.rst` should look like this: | ||
``` | ||
.. toctree:: | ||
:maxdepth: 1 |
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.
It's not necessary to specify maxdepth. The same applies to other lines.
Co-authored-by: Junye Huang <[email protected]>
This PR adds missing chevron svg file used in footer Previous/Next buttons (original image accidentally removed in #138 ). It also updates references from `chevron-right-orange` to `chevron-right-purple`, matching the fill colour used. Updated footer rendering: ![image](https://user-images.githubusercontent.com/25207344/228622405-0e8a0ac2-f1b2-44af-be2c-85ee4ac48d52.png) Fixes #189
This PR adds missing chevron svg file used in footer Previous/Next buttons (original image accidentally removed in Qiskit#138 ). It also updates references from `chevron-right-orange` to `chevron-right-purple`, matching the fill colour used. Updated footer rendering: ![image](https://user-images.githubusercontent.com/25207344/228622405-0e8a0ac2-f1b2-44af-be2c-85ee4ac48d52.png) Fixes Qiskit#189
) This PR adds missing chevron svg file used in footer Previous/Next buttons (original image accidentally removed in #138 ). It also updates references from `chevron-right-orange` to `chevron-right-purple`, matching the fill colour used. Updated footer rendering: ![image](https://user-images.githubusercontent.com/25207344/228622405-0e8a0ac2-f1b2-44af-be2c-85ee4ac48d52.png) Fixes #189 Co-authored-by: Karla Spuldaro <[email protected]>
fixes #72 , fixes #70
This PR restyles the sidebar and makes it configurable for users of the theme in the following way:
version_list
.translations_list
.qiskit_sphinx_theme
README with a recommended list of sidebar headingssidebar_headings
variable in theirconf.py
. This is also now explained in the READMEOther work in this PR:
Tasks: