-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add version switcher, favicon, and context-dependent logo #586
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #586 +/- ##
=======================================
Coverage 93.13% 93.14%
=======================================
Files 60 60
Lines 4503 4506 +3
=======================================
+ Hits 4194 4197 +3
Misses 309 309 ☔ View full report in Codecov by Sentry. |
Amazing Nich!! Favicon is a sweet plus. Two things:
I'm not fully sure how to solve 1., I think we need to find a way to control the width of the 3 navbars/zones (left, middle, right)? Can the version switcher be made less wide by changing the default text from "choose version" to "vesion" or simply "stable" by default? The "search" box also takes a lot of space. |
In response to the review I
I believe I addressed all your comments, @maximelucas! |
@@ -184,6 +201,13 @@ | |||
"icon": "fa-brands fa-mastodon", # Font Awesome icon | |||
}, | |||
], | |||
"header_links_before_dropdown": 4, |
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.
Any way of having 5 and keeping the Gallery visible (not in dropdown)?
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 issue was that I wanted everything on one line even when the user wasn't in full-screen, which is basically impossible unless the navbar is slimmed down. So I will keep it like it is for now, and then change it later if need be.
You're on fire, the automatic date retrieval is great!! |
Co-authored-by: Maxime Lucas <[email protected]>
Thanks for the review! |
Fixes #584 and #585.
This PR