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

Fix navbar alignment and sidebar scrollbar #12571

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Apr 24, 2024

Fixes the issues mentioned in #12558 regarding the navbar getting rendered across two rows as well as a scrollbar in the sidebar getting rendered although there is enough vertical space.

@cbrnr cbrnr changed the title Fix navbar alignment Fix navbar alignment and sidebar scrollbar Apr 24, 2024
@hoechenberger
Copy link
Member

@cbrnr Nice, looks good on my system. I still get a horizontal scrollbar in the left navbar, any chance to get rid of this one too?

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 24, 2024

I still get a horizontal scrollbar in the left navbar, any chance to get rid of this one too?

Maybe setting the left and right margins to zero works? I'll give it a try, but you have to test it then, because I don't see a horizontal scrollbar on my system.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 24, 2024

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

I don't love the look of navbar_align="left" but if others are regularly seeing the top nav elements word-wrapping then fixing that is more important than my aesthetic preference.

Still needs green CIs though, and @hoechenberger's confirmation that the horiz. scrollbar is gone too

@hoechenberger
Copy link
Member

Hey, this fixes the issue, but I agree with @drammock that the (too) left-justification is not the most aesthetically pleasing solution. Could we add more padding between the logo and the first navbar element? Or center the elements – or was that what was causing the issues in the first place?

@drammock
Copy link
Member

There was just a new PR opened into the theme that addresses the menu item wrapping: pydata/pydata-sphinx-theme#1784

Might be worth waiting for that?

@drammock
Copy link
Member

Or center the elements – or was that what was causing the issues in the first place?

what was causing the issue was navbar_align="content" AKA align first navbar item to the border between left sidebar and main content. The theme doesn't have a "center them" option.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 25, 2024

Might be worth waiting for that?

I tried this PR, and it looks better than the align_left "fix", although at medium widths, there are still two rows in the navbar:

Screenshot 2024-04-25 at 17 14 47

But at least the menu items are not wrapped. Maybe it would be better to just have two layouts, a narrow and a wide one and not a third variant in between. But yes, let's wait for that PR to get merged.

@hoechenberger
Copy link
Member

hoechenberger commented Apr 25, 2024

I tried this PR, and it looks better than the align_left "fix", although at medium widths, there are still two rows in the navbar:

Screenshot 2024-04-25 at 17 14 47

That looks good to me under the given circumstances

"Normally" one would expect some navbar elements to be moved into a hamburger menu on smaller screens

I think one could save some horizontal space by shortening the "Choose version" label to just "Version"

@drammock
Copy link
Member

Maybe it would be better to just have two layouts, a narrow and a wide one and not a third variant in between.

This feedback really belongs on the theme repo, not here.

"Normally" one would expect some navbar elements to be moved into a hamburger menu on smaller screens

I'm not sure what you mean by putting "normally" in scare quotes here? Anyway, the hamburger menu does happen. But the theme cannot know a priori how many menu items each site will have, or how long their names will be. So it is always possible for edge cases to arise where the theme's breakpoints don't align well with the widths of the site content. This is mitigated somewhat by providing an optional "More..." dropdown with a configurable threshold, as a way to adjust the natural size of the topbar menu items to the breakpoint.

I think one could save some horizontal space by shortening the "Choose version" label to just "Version"

This feedback also would be more helpful if it were raised on the theme repo, not here.

@cbrnr
Copy link
Contributor Author

cbrnr commented Apr 30, 2024

To summarize, this PR (1) fixes the vertical scrollbar issue in the sidebar and (2) improves (not fixes completely) the navbar (items are in a single row for reasonable widths).

I cannot reproduce the horizontal scrollbar issue at all, so I suggest that this should be reported/added to pydata/pydata-sphinx-theme#1238.

If you want to wait for pydata/pydata-sphinx-theme#1784, I can remove that part of the PR, so that it only includes the vertical scrollbar fix. WDYT?

@drammock
Copy link
Member

Ok with me to merge / not wait. But some CIs are still failing

@hoechenberger hoechenberger merged commit e3a40fc into mne-tools:main Apr 30, 2024
30 checks passed
@hoechenberger
Copy link
Member

thanks @cbrnr

@cbrnr cbrnr deleted the navbar branch April 30, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants