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 (responsive) navbar dropdown menu #313

Merged

Conversation

jorisvandenbossche
Copy link
Member

Closes #304

@@ -3,7 +3,6 @@
height: var(--header-height);
width: 100%;
padding: 0;
overflow: hidden;
Copy link
Collaborator

Choose a reason for hiding this comment

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

where did this addition come from? we should make sure it wasn't fixing a different bug or something

Copy link
Member Author

Choose a reason for hiding this comment

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

It was added in #286 (comment), but I don't directly know why it would have been necessary (I tested a few of the targets that #286 was fixing, and they still seem to be working)

@choldgraf
Copy link
Collaborator

argh it seems like the RTD hook to auto-build the documentation isn't working?

@jorisvandenbossche
Copy link
Member Author

argh it seems like the RTD hook to auto-build the documentation isn't working?

It was still enabled in the settings, and there was actually also a build for this PR, only didn't show up here. Seems that triggering it again by reopening the PR fixed that ;)

@choldgraf
Copy link
Collaborator

Hmmm, I think we need to add a background to the dropdown. Here's how it looks in the RTD build:

image

@jorisvandenbossche
Copy link
Member Author

jorisvandenbossche commented Feb 5, 2021

Hmm, so it seems that's because we now set a fixed height for the navbar, but of course in mobile mode when toggling the navbar needs to be able to become bigger.
There might be a way to let the height expand just in mobile mode, but setting the fixed height only on the navbar-brand (to limit the size of the figure) also seems to work .. (but not fully sure this is a good fix).

@jorisvandenbossche jorisvandenbossche changed the title Fix responsive navbar dropdown menu Fix (responsive) navbar dropdown menu Feb 5, 2021
@drammock
Copy link
Collaborator

TL;DR: would be great to get this merged, it's a blocker for MNE-Python using this theme. Read on for idea about an alternate implementation that works on our site but may not generalize well.

Wondering what the status of this PR is... personally I am content with the approach here, although I've also been playing around with adding a background color and classes offset-9 and offset-lg-0 to the #navbar-menu node, and reverting the change in .navbar from min-height back to height. That way, the hamburger menu drops down from the button at the right edge (under the hamburger) instead of having the nav items appear on the left side and the hamburger on the right.

The downside to my approach is that the navbar-menu node would also need a shadow or a border to distinguish it from the content that it overlays, and (more seriously) I'm not sure how to programmatically pick the amount of offset that is appropriate for the width of the menu content (I thought col-auto combined with justify-content-end might work, but I couldn't get it to).

@drammock
Copy link
Collaborator

I've also been playing around with adding a background color and classes offset-9 and offset-lg-0 to the #navbar-menu node, and reverting the change in .navbar from min-height back to height.

after playing around with my proposed solution some more, I'm not convinced that it's workable and hence I would advocate for merging this PR as-is. It seems to fix the problem and seems perfectly fine from a usability perspective. It would maybe be prettier if someone could figure out how to auto-shrink the width of the hamburger dropdown and anchor it to the right side under the hamburger button, but I at least couldn't find a way to make that work cleanly.

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.

BUG: mobile dropdown is not working anymore on master
3 participants