Remove the animation when “docking” the menu

[ryelle]

I was chatting with @melchoyce about the menu docking on scroll, and we thought this was a smoother (less distracting) experience -- no bounce animation, and the menu stays the same height.

wp.org username: ryelle

[ryelle]

Doesn't work as expected with a custom header image (which I couldn't get to work before due to having a blog on front) — will update tomorrow with a fix for this, too.

[ryelle]

Fixed the custom header issue — this is now ready for review.

[josephfusco]

@ryelle After testing locally I am running into an issue where occasionally the menu stays fixed when scrolling back up. It doesn't happen every time, but if i scroll too fast it seems, it will behave like this.

[ryelle]

How strange. I was able to reproduce it, and it looks to be caused by the site-navigation-hidden class - which looks redundant. I've removed it in this latest commit, which seems to fix that.

[josephfusco]

Working great now :) We just need to account for the 1px border. I am noticing a subtle shift when the menu docks because of it.

[josephfusco]

To include border in height this should be

$navigationHeight = $navigation.outerHeight();

[josephfusco]

and also on line 48 within adjustHeaderHeight()

[allancole]

Just noticed that this sort of breaks on shorter pages.

As you scroll down the site and the menu hits the top of the browser, it attempts to fix the header to the top of the window. On most pages this is fine, but on shorter pages when the menu is fixed, the page height gets decreased which makes the entire site fit inside of the browser window. When that happens it creates an impossible situation where the header wants to be both fixed at the top and in-line with the header at the same time.

Here’s a rough screen cap: https://cloudup.com/cCBpdOsHhSv

Best to try this out on your own. To replicate, create a page with just enough content to match the height of the entire header (with in-line menu). Then try and scroll down and you’ll see the how the menu gets confused and starts to jump around.

[ryelle]

Switched to outerHeight to include the border. Thanks @josephfusco :)

@allancole I can't replicate what you're seeing. The page height doesn't change for me. What OS/browser are you using?

[josephfusco]

@allancole I am able to reproduce that on master branch but not with this PR. You are using https://github.com/ryelle/twentyseventeen/tree/menu-animation ?

[allancole]

@ryelle, Yeah it’s tough to explain properly, but I’ve run into a similar issue before in the past.

I’m using the Chrome browser. But the issue also happens in Safari. See here: https://cloudup.com/cnvprFHlX0Y

One thing I left out of the replicating instructions is that comments should be turned OFF, so that the page height is as short as possible.

[allancole]

@josephfusco Yup, I pulled the code down from this PR.

git fetch origin pull/151/ryelle:menu-animation

[ryelle]

Just to make sure - you did both steps, fetching the branch and checking it out?

git fetch origin pull/151/head:menu-animation
git checkout menu-animation

[allancole]

Yup, I ran both of those commands. I can see the other improvements from this PR just fine, but this post (and one other like it) seem to show this one problem.

Maybe if you download and import the theme unit test, you can take a look at that same “Clearing Floats” page and see if you see the same issue?

[josephfusco]

@allancole I am using the "Clearing Floats" page of the theme unit test, removed footer widgets, added 3 rows of menu items, and am still not able to reproduce with this PR.

I have looked at other pages too with shorter heights and played with the browser height trying to hit that sweet spot where scrolling ends and the menu just starts to hit the top of the viewport.

🤔 Possible cache? I know I already said this but this behavior is identical to master in the same situation.

macOS 10.12
Chrome 53.0.2785.143

@davidakennedy Do you mind giving this a shot with trying to reproduce the behavior @allancole has mentioned? This is the only remaining issue I can see here.

[allancole]

So odd that it isn’t happening for anyone else. I cleared my cache but it didn’t fix this for me.

I’m on a 13in MBP so maybe screen size (or browser height) has something to do with it. Maybe try adjusting the height of the browser to make it smaller? I haven’t looked at the code, but since this only happening for me right now, maybe I can make a separate ticket for this and submit my own PR—assuming I can figure out a fix :-)

[ryelle]

Nothing new added ^, just rebasing on master.

[grappler]

@ryelle The travis test is not passing.

[ryelle]

Fixed the jscs issue, travis should pass now.

[josephfusco]

Since we can't reproduce the edge case issue, I think this is ready to be merged and we can get more people using it to see if it comes up with anyone else.

maybe I can make a separate ticket for this and submit my own PR

@allancole that sounds good once this is in master if it is still happening on your end.

[melchoyce]

Tested the PR and it looks good to me. Since it's a bigger one, I'm going to leave for @davidakennedy to take one last look at tomorrow.

[davidakennedy]

Thanks for the pull request @ryelle and for the excellent testing @josephfusco!

@allancole I couldn't reproduce your issue, even after trying a feel "short" pages and resizing my browser to all kinds of heights. I'll keep this in mind as the theme is tested more though – just in case.