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 sidebar scrolling #2860

Merged
merged 8 commits into from Apr 21, 2017

Conversation

Projects
None yet
5 participants
@allejo
Contributor

allejo commented Apr 18, 2017

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 used wilddeer/stickyfill and 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

@mistyhacks

This comment has been minimized.

Contributor

mistyhacks commented Apr 18, 2017

Ping when Netlify finishes and I'll have a look.

@mistyhacks

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.

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 18, 2017

Sweet, I'll clean/fix this up later today

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 19, 2017

@mstanleyjones does the latest netlify preview fix the issue you were having?

@mistyhacks

This comment has been minimized.

Contributor

mistyhacks commented Apr 19, 2017

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.

allejo added some commits Apr 19, 2017

Simplify both left and right sidebars
- 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
Improve the mobile menu to be fixed
- 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`
Fix disappearing top navbar on homepage
- 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
@mistyhacks

This comment has been minimized.

Contributor

mistyhacks commented Apr 19, 2017

I kicked Netlify.

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 19, 2017

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 👍

@mistyhacks

This comment has been minimized.

Contributor

mistyhacks commented Apr 19, 2017

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.

@mistyhacks

This comment has been minimized.

Contributor

mistyhacks commented Apr 19, 2017

@johndmulhausen WDYT? I think this is quite an improvement. Are these changes in the right places so that @jsouth won't wipe them out?

@mistyhacks mistyhacks requested a review from johndmulhausen Apr 19, 2017

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 19, 2017

Not quite yet. There is a problem with the affixed header disappearing on mobile. Almost there!!

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?)

@stevenhanna6

This comment has been minimized.

Contributor

stevenhanna6 commented Apr 20, 2017

All pages
Chrome, Firefox, Edge: vertical scrollbars always visible in left and right sidebars (before they were hidden until needed)

Homepage
Edge: left and right sidebars do not move down when scrolling

Other pages
Chrome, Firefox, Edge: right side bar horizontal scroll bar always visible (was hidden before)
Edge: left and right sidebars do not move down when scrolling

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.

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 20, 2017

Fixed scrolling in the overflow in 7d01ffc. IE + Edge doesn't support position: sticky yet so I'm waiting on a yay or nay on using a polyfill and which one if it's a yay.

@stevenhanna6

This comment has been minimized.

Contributor

stevenhanna6 commented Apr 20, 2017

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.

@johndmulhausen

This comment has been minimized.

Contributor

johndmulhausen commented Apr 20, 2017

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:

img

The fix: As soon as you decide to hide that left nav, the hamburger (the left nav's substitution) needs to pop in.

@stevenhanna6

This comment has been minimized.

Contributor

stevenhanna6 commented Apr 20, 2017

@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.

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 20, 2017

@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)

Update: Homepage isn't fixed for the hamburger menu because it uses a different custom nav. Currently looking at this. see #2860 (comment)

Fix left nav hidden after expanding from mobile
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
@stevenhanna6

This comment has been minimized.

Contributor

stevenhanna6 commented Apr 20, 2017

@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.

@allejo

This comment has been minimized.

Contributor

allejo commented Apr 21, 2017

I've unified the nav bar on the homepage and the rest of the website with a single include in b307c2c; this should fix the missing hamburger on the homepage.

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?

@jsouth

This comment has been minimized.

Contributor

jsouth commented Apr 21, 2017

So awesome to see all of these improvements taking place!

Thank you rockstars for helping us tighten this up.🌟🌟🌟

@johndmulhausen

This comment has been minimized.

Contributor

johndmulhausen commented Apr 21, 2017

@allejo This is just excellent work. Thanks so much for being responsive and for helping us with this, it's a big one.

@johndmulhausen johndmulhausen merged commit f466f24 into docker:master Apr 21, 2017

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
deploy/netlify Deploy preview ready!
Details

@allejo allejo deleted the allejo:2403-patch branch Apr 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment