-
-
Notifications
You must be signed in to change notification settings - Fork 3
Update sidebar and footer navigation behavior to regain screen real estate #20
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
Conversation
|
Title size is based on Rubicon Objective C fitting on two lines. The rest of the changes should fix the issues mentioned here, within reason. Tested with Tutorial build. |
|
@freakboy3742 This is ready for review. |
freakboy3742
left a comment
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.
Definitely an improvement. A couple of nits that I found:
- The version number seems misaligned with the title when the screen width is > 1600px
- The gray background on the secondary bar on small screens is a good touch, but probably a bit dark.
light-dark(#f8f9fb,#1a1c1e)is what the current theme uses on the sidebar; that's a bit less strong, but still visually distinct. - To that end - could we add that background color to the wide-mode sidebar as well (to match the current BeeWare theme)?
- In light mode, at around 860px, the tray icon on the secondary title bar goes white.
- Something I didn't spot before, but is a bug: the Source and Issues links aren't in "link blue".
- When in narrow modes, on a non-home page, the site icon renders "text beside icon". On the home page, the icon is small, but the text renders below. Given screen real estate is limited, I suspect we want to keep the "text beside" version, even on the home page, rather than use a larger icon on the home page.
The only thing I can't easily check is to see how much the changes to the footer and logo improve index usage. It might be worth adding a dummy section 3 that contains a dozen or so children so we can test that on the docs preview?
I'd like to add (I know, should've found more issues bee-fore posting my previous comment) that the Also I have no idea if I'm being weird or something, but at https://beeware-docs-tools--20.org.readthedocs.build/en/20/section_one/page_two/ the Next navigation seem to take up less space than the Previous one. Not sure if this is a bug though. |
I'm not seeing this issue on the rendered version or the live version I'm working with locally, in any browser. |
Likewise - I'm not seeing this either. |
|
Hmm... I've forced a refresh and it's still like the following... but no pressure to find out what's going on, maybe fix later after we get more reports of this. Screen.Recording.2025-08-25.at.9.37.26.PM.mov |
The dark bar seemed a bit too dark to be visually distinct in dark mode. I went with a lighter color.
Not really. It is not any wider than its contents plus a little padding; it does not extend to the edge of the display. Further, the fixes for regaining screen real estate on scroll mean that it wouldn't go all the way up to the navbar. If you really want this feature, it'll involve overriding more template partials than we already are, possibly |
|
Yes I'm aware that my "fix" for the RTD ad broke the footer and sidebar interaction. Working on it. |
|
Confirmed the bouncing problem with the footer has gone. I'm still seeing a minor misalignment on the version number on non-home pages, when the width is 1220+ and 1600+. The secondary toolbar background colors are fine by me; it's a pity we can't get that actually in the sidebar, but short of a major redesign, I don't see a way to set the background color either. If you can find a way to do it that isn't overly invasive, it would be a nice to have; but I can live without this one. All the other issues in my last review look fixed. I can confirm I'm seeing the on-hover color for the "Made with MkDocs" issue that @johnzhou721 reported. The wandering ad placement thing is also still happening; the only lead I've got on that is that appears something is setting an explicit height on the sidebar. From what I can tell, on the pages that are affected by the wandering ad, the ad is being after that height is being applied. It looks like there's 2 dynamic processes in play - one setting the height of the right sidebar, and one inserting the ad - and if the height of the sidebar is set before the ad is inserted, RTD determines the ad won't fit, and puts it somewhere else. What I can't understand is why something would be setting an explicit height on that sidebar element at all. |
|
I decided we're going about this real estate on smaller height display things all wrong. I can use CSS to hide the navigation bar below a certain height. I have added this feature at 640px. Let me know if this is something you want, and if so, what height you want it to disappear. Basically, if the page is short, it won't show. I removed the
This is coming from the Material theme itself. I added an override to turn it link-blue on hover, but nothing I did changed it to begin with.
Not without a major redo of the overall content container, and even then, I'm not sure how I'd manage the dynamic size changes. I considered showing you how it would look with only the sidebar a different color, but the fixes to regain screen real estate makes that impossible without gaps above and/or below the sidebar. tl;dr: It's not happening.
I put the ad
I spent the better part of the last three and a half hours chasing this down. It's happening here as far as I can tell. I created an empty version of that file in the same directory structure, but it doesn't appear to override it, which I guess makes sense. I commented out the Setting |
To my mind, the vertical real estate issue has been almost entirely addressed by moving the ad, and made even better by the smaller logo on non-home pages, so I don't think any additional hacks/workarounds are needed here.
It seems to have prevented the ad from migrating... but the ad is now not displaying at all on /section_two/ or /section_one/page_two/. That's substantially better than the "mid index" position, so I can live with it.
Ok - so it's a deeply embedded part of the material theme. If that's the case it might be worth reporting upstream so they're aware of it, but it's not worth fighting it. We can only cook with the ingredients we've been given, and while using JS for styling makes me a little twitchy, it clearly works well enough, and I don't have anywhere near enough enthusiasm to build and maintain our own theme from scratch. |
freakboy3742
left a comment
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'm happy with the current state of this - or, at least, happy enough that the need for any more changes will become apparent in the context of the tutorial or Toga, for which these changes need to be landed.


It was noted on the Toga PR that on pages with a short amount of content, the footer navigation interferes with the sidebar ToC list and makes it difficult to navigate. The following changes address this issue:
/).If this has not regained enough space, it is possible to force the footer nav entirely out of the viewport; please let me know if that is what is preferred.
Additionally the demo site has been updated to address verifying the changes to the features, and an unused plugin has been removed from dependencies.
These changes have been tested with the current Toga build.
PR Checklist: