Fix sidebar scrolling #2860
Fix sidebar scrolling #2860
Conversation
|
Ping when Netlify finishes and I'll have a look. |
There was a problem hiding this comment.
This is really close, but it changes the alignment of the right-hand TOC when changing from the global header to the fixed header, at least on my screen. Scroll down the page a little and you will see it happen. Actually what I think is happening is that the content div is getting just a little wider when we go to the fixed header.
|
Sweet, I'll clean/fix this up later today |
|
@mstanleyjones does the latest netlify preview fix the issue you were having? |
|
Hmm, not quite yet. We miss the sticky header at the top. Go to https://docs.docker.com and scroll down a bit, and notice that it sticks to the top. |
- Rewrite the sidebars to use the same CSS class instead of having two very similar classes. This involves removing all affix related attributes and functionality from the sidebars and replaced them with `position: sticky`. - The table of content elements should not be floated - Removed unused CSS rules related to the sidebars - Remove JavaScript used to resize the sidebars
- Simply JS used to hide/display the navigation bar on mobile viewports - The mobile menu should be fixed so it can be viewable from the bottom of the page. Before this change, you would have to scroll up to see the menu that `position: absolute`
- Change the calculation of the affix offset for the navbar on the home page to ensure the navbar never leaves the viewport. This change also takes into account the change of how the sidebars' offsets are used
|
I kicked Netlify. |
|
All of my commits have been squashed and cleaned up to have meaningful messages. Netlify has the latest preview of this PR. I've also updated the first post. Feedback and issues welcome |
|
Not quite yet. There is a problem with the affixed header disappearing on mobile. Almost there!! Actually that's not your fault. It's already there in the live site. |
|
@johndmulhausen WDYT? I think this is quite an improvement. Are these changes in the right places so that @jsouth won't wipe them out? |
Since the affixed header behaves the same way as the production site, i reported the mobile navbar issue it in #2938 (I've got a fix in mind, I can patch it in this PR or a separate one, any preference?) |
|
All pages Homepage Other pages My solution doesn't have these problems but your solution is quite different so I can't offer any help. I guess the edge browser bugs are the main ones to be fixed, the others and more visual differences on how the site behaved before. |
|
Fixed scrolling in the overflow in 7d01ffc. IE + Edge doesn't support |
|
Just checked your scrolling overflow fix, it fixed the scrollbar issues. Great! Can't wait to see how you fix the Edge bug. I think your overall solution is much cleaner then the checkNavSizes() and affix usage. Well done. |
|
Loving the left nav resize logic -- the scrollbars and affix logic is looking good. The problem right now is that we're losing the hamburger menu at the top-right for a large swath of resolutions -- and without that, you can't navigate the site. Watch how narrow it has to get before the hamburger menu comes back in the top right, as I gradually narrow the viewport: The fix: As soon as you decide to hide that left nav, the hamburger (the left nav's substitution) needs to pop in. |
|
@allejo your pollyfill trick worked on Edge, looks good to me. Cleared up all the issues I saw @johndmulhausen looks like that bug is in the original site and wasn't introduced by allejo, there is also a bug I just noticed on the original site. Click the hamburger, close it, then reisize your browser back to get rid of the hamburger...the left nav panel is now blank. |
|
@stevenhanna6 woo! glad that works @johndmulhausen as Steven pointed out, I'm not to blame for the issue (whew!) but fixed it in this PR none the less (b459ade). I also fixed the left nav being blank after expanding from the mobile view (ef16138)
|
If you expand and collapse the left nav while on mobile and expand to a desktop view, the left nav would be hidden; this has been fixed
|
@allejo Just want to confirm same thing you see, just the hompage has that one last hamburger bug. Everything else works perfectly, if you can't figure out that last bug I would suggest breaking it out into a separate issue so the rest of your work gets looked over and merged. To me its a huge improvement over the original code. Plus the hamburger stuff wasn't part of this issue. |
|
I've unified the nav bar on the homepage and the rest of the website with a single However, the nav bar as a whole really needs some TLC, which would be better suited for its own PR/project. @johndmulhausen mind taking a look? |
|
So awesome to see all of these improvements taking place! Thank you rockstars for helping us tighten this up. |
|
@allejo This is just excellent work. Thanks so much for being responsive and for helping us with this, it's a big one. |
Proposed changes
Move away from a JS solution to keep the sidebars in place and use a more robust CSS solution to take advantage of
position: sticky. A polyfill will be necessary to support IE+Edge, which are the only modern browsers lacking support. I've previously usedwilddeer/stickyfilland has worked for me, however I don't know if there's a different one you'd like to explore (modernizr has a polyfill as well).Related issues (optional)
Fixes #2403
Fixes #2232