-
Notifications
You must be signed in to change notification settings - Fork 310
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
Making navbar + a few other sections more modular #355
Conversation
I have tested it locally and can confirm that the default look is not altered and I can use the |
How about adding a version switcher instead of just display it in the top nav-bar? (#350 ) We can put a |
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.
Cool! Fully on board with the principles, thanks!
Removes the configuration value to put the search in the sidebar or the navbar
Do you know if there is an easy way to deprecate this first?
docs/conf.py
Outdated
from pathlib import Path | ||
|
||
for ii in Path("../pydata_sphinx_theme/__init__.py").read_text().split("\n"): | ||
if ii.startswith("__version__"): | ||
version = release = ii.split("= ")[-1].strip('"').replace("dev0", "") |
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.
You could also import the package and get pydata_sphinx_theme.__version__
?
docs/user_guide/configuring.rst
Outdated
|
||
Generally speaking, navbar menu items will be **right-aligned**. However, the | ||
``navbar-menu-nav.html`` template any anything that comes before it will | ||
be **left-aligned**. |
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 am wondering here: does this signal that we should rather split it in three? And also have a navbar_right
option for the content that is right-aligned?
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 was wondering the same thing, but wasn't sure if I had the right CSS magic to make it happen. Do you have a suggestion for the changes we'd need to make to do this? I agree that it'd be more sensible if there were:
navbar_left
- left side of screen, left-justifiednavbar_middle
- aligned to content, or left side of screen, left-justifiednavbar_right
- right side of screen, right-justified
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.
Hmm, right now the menu items and icons are in the same div, using a single class/alignment. So that's indeed a bit a bigger change to be able to split those.
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.
We still need to ensure that the middle and right are together in the collapsible div, so that the mobile responsiveness is correct.
I did a quick attempt, see the commit I pushed. It seems to work the same as it does now (but didn't check if the alignment is still correct if you are using multiple custom templates)
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Thanks for the review @jorisvandenbossche - some quick comments above. For this one:
Good point, what if we kept the flag, and for now if that flag exists it will manually update the nav configs to put the search template in the right place? And raise a dep warning? |
That sounds good to me |
@jorisvandenbossche ah nice - this almost gets there, but if you add extra items in addition to the menu, this happens: any idea what's going wrong there? EDIT: Ah I figured it out - just float lefts and rights: |
OK I think I've addressed all comments, added a deprecation, fixed the CSS styling, and got the tests working. Anything else to take care of? @jorisvandenbossche I didn't change the |
@jorisvandenbossche what do you think about these two extra additions I can add to this PR:
Do those seem reasonable? I think this would then allow for most major parts of the page to be extended / configured in this way, rather than using feature flags for everything |
Sounds good to me. Just wondering, for the footer, I would think you will typically override the full footer? In which case you could also add a |
OK @jorisvandenbossche - I've added in a https://pydata-sphinx-theme--355.org.readthedocs.build/en/355/user_guide/sections.html |
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.
Cool, thanks a lot for the updates! Good idea for making a separate doc page about it
I added a bunch of comments. Large part of them are pedantic naming discussions, but since we let people override those based on the name, we should stick with the names once we introduce them, so a bit of bikeshedding can be useful then ;)
docs/conf.py
Outdated
# "navbar_align": "right", # For testing that the navbar items align properly | ||
# "search_bar_position": "navbar", # TODO: Deprecated - remove in future version | ||
# "navbar_align": "left", # [left, content, right] For testing that the navbar items align properly | ||
# "navbar_left": ["navbar-logo", "navbar-version"], |
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 maybe fine to include the version in our demo docs? (it can't harm to include I think, and then it's directly a test that part of this works in the online docs)
docs/user_guide/sections.rst
Outdated
- Left section: ``html_theme_options['navbar_left']`` | ||
- Middle menu: ``html_theme_options['navbar_menu']`` | ||
- Right section: ``html_theme_options['navbar_right']`` |
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.
For the actual variable names, should we maybe use "start" and "end" instead of "left" and "right"? (it's eg also what bootstrap starts to use more: https://getbootstrap.com/docs/5.0/getting-started/rtl/#approach)
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.
And maybe "navbar_center" instead of "navbar_menu"? (as you are free to put whatever element in there)
docs/user_guide/sections.rst
Outdated
... | ||
"navbar_left": ["navbar-logo"], | ||
"navbar_menu": ["navbar-menu-nav"], | ||
"navbar_right": ["navbar-menu-buttons"] |
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.
Another naming suggestion:
"navbar_right": ["navbar-menu-buttons"] | |
"navbar_right": ["navbar-menu-icons"] |
or "icon-links" instead of "icons" to be fully consistent with the theme option that populates them
@@ -0,0 +1,3 @@ | |||
<p class="last-updated"> |
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 original had a if last_updated
, should that be kept?
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 was thinking that people could just provide this in the footer config if they wished, rather than using the Sphinx feature flag. Does that make sense?
I've added a docs section with a list of templates that can be inserted in. This isn't super descriptive, but I think should help people know what they can put in here. Does that work?
src/scss/_navbar.scss
Outdated
float: left; | ||
} | ||
|
||
// Anything in navbar menu should be left-justified (e.g. links) |
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.
// Anything in navbar menu should be left-justified (e.g. links) | |
// Anything in right part of the navbar should be right-justified (e.g. icons) |
Thanks for these comments @jorisvandenbossche - I implemented pretty much all of them (w/ one exception which I responded in-line above). Great points about naming things, I think "start" and "end" is much better. Let me know if there's anything else I should finish up (and maybe have a quick look that I didn't miss anything in the renaming) |
Thanks for the updates! I pushed a small fix to add again a bit of padding at the bottom of the footer (which was removed by removing the padding around the |
This is an attempt at making the navbar more modular, so that it can be more easily customized. It does a couple of things, though the end-result should look the exact same as what we have right now.
It defines two new configuration:
html_theme_options: {"navbar_left": ..., "navbar_menu": ...}
Each of these are a list of templates, similar to
html_sidebars
, that will populate the two sections of the navbar.Removes the configuration value to put the search in the sidebar or the navbar, since people can do this via
html_theme_options: {"navbar_menu": ["search-field.html"]}
now.Splits our navbar HTML into a few components in
_templates/
Adds an
ethical ads
templateTries to document a bit of this, and adds a little
version
template for the docs so that we can see how one can extend the left navbar area.@jorisvandenbossche what do you think about this? If you're +1 then I can add some tests.
The docs previous shouldn't look any different, but just for reference: https://pydata-sphinx-theme--355.org.readthedocs.build/en/355/user_guide/index.html
(the one difference is that there should now be a little version next to the logo)