Update URL before animation

[agarzola]

Instead of waiting for the animation to stop before updating the URL, let’s update it right off the bat.

I’m using nav:target to show/hide my main menu on touch devices using only CSS. One of the benefits of this is that as soon as a menu item is tapped, the menu is hidden because the URL changes. Enter smooth-scroll: Because the script waits until the animation stops before updating the URL, the menu stays visible while the script animates the scroll to the selected location, and only then closes. This looks awkward.

This commit changes the order of things, so that the URL is changed immediately, making my menu toggle at the expected time (immediately after tapping a menu item).

Let me know if you would prefer a more robust solution that offers both options. For example: dataURL could have the values before, after, and false, so developers have more control over this behavior. I’d like to take a crack at that if it’s something you’d like to see.

Thanks!

[agarzola]

I just noticed that there’s a very brief flash after the click and before the animation. This is true for both Chrome and FF. I imagine it’s because changing the url makes the browser want to skip to that anchor, and then the script comes in and brings it back to where it was as it begins to animate.

I’ll look into this some more and work on a solution.

[agarzola]

Found a solution. We’re already testing for history.pushState on line 53, so there’s no need to use window.location.hash to cater to older browsers, because they’ll test false anyway.

Solution now uses pushState and no more jerkiness. I’d still like to pursue the before/after/false option I mentioned above, so let me know if that’s something you’d like to see. Thanks!

[agarzola]

Turns out there’s a bug that makes Webkit browsers ignore :target when a new hash is added via history.pushState instead of window.location.hash. The same behavior is observed in Firefox.

I’m reverting to using the original behavior (update URL after animation) until I can find a better solution, so I’m closing this pull request.

[cferdinandi]

Hi @agarzola - Sorry for the delayed response on this. As @robertpateii notes in another issue, there are some issues with the URL update and back button behavior in FireFox with the current implementation, so I'll be looking into that a bit more closely. I do like your before, after, and false options idea, though. I'll keep you posted!

[agarzola]

Thanks @cferdinandi! No worries. I’m still looking into something that works for me, and then will report back.

For the time being, I’m keeping the update-URL-before-animation behavior and making the following adjustments to the updateURL function:

// Function to update URL
var updateURL = function (url, anchor) {
  if ( url === 'true' && history.pushState ) {
    window.location.hash = '#top';
    history.replaceState(null, null, '#' + anchor.id);
  }
};

This changes the location hash to #top, (that’s my header, which is fixed, so no jump is perceived) and then immediately replaces it with the history API to the desired anchor. So I get the best of both worlds: I remove the :target CSS from my menu (which hides it), and I get the anchor hash added to the URL without any jumps.

This solution is, of course, very specific to my needs, but I thought I’d share just in case there’s anything in there that sparks any ideas on your end.

Thanks again!

[robertpateii]

Hey guys,

Yeah, to support before behavior we'd have to go back to using history (which would fix #26). We can't simply update window.location.hash prior to scrolling as that will move you immediately to the target.

I'm happy with before being the default behavior. Don't even offer the after option. It actually makes more sense to do it before scrolling. Clicking a link should update the URL bar immediately.

(But if you must have all 3 options, may I suggest you make them before | after | off instead? That would be less confusing than having what appears to be a boolean but is actually a string.)

Best,

Rob

[cferdinandi]

@robertpateii - I agree on both points. I think you're right: it should be at the start or not at all. Cheers!

[robertpateii]

I would suggest one change to the code in this pull request. Use the pushState line from my commit in #18. We should avoid passing null in the state and title arguments.

Null is the default state object, so you don't want to re-issue that when you're actually in a different state. And the [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/Manipulating_the_browser_history#The_pushState(\).C2.A0method) suggests that the 2nd argument, title, should be set to an empty string because we're not sure how it will be used in the future.

so replace this:
history.pushState(null, null, '#' + anchor.id);

with this:
history.pushState({pos:anchor.id}, "", '#' + anchor.id);

Would you agree?

p.s.
I chose pos there, but it could be anything. I couldn't find a standard.

[cferdinandi]

@agarzola - This has been fixed in version 2.18. Thanks for your help on this!

[agarzola]

Great job, @cferdinandi! Thanks again.

[cferdinandi]

Also, sorry for not merging your pull request. Because I was juggling this request and #26 at the same time, I ended up updating the code manually. Appreciate your help greatly, though!

[agarzola]

My ego is intact! I’m just happy that I get to use this script. Thank you for sharing it with the world.