Accept nonstandard links

[charlesbaynham]

Issue

Currently skrollr-menu will not animate / deal with any hashlinks to the current page unless they are expressed with href='#xxx'. This means that absolute links to the current page with hash-links will fail when clicked from that page.

Example

The menu of a site has a link with href='/#mysection'. This will fail because the preceding / causes skrollr-menu not to parse it, even though it should be considered a valid link.

Changes

This commit addresses this by including a function to check if a link points to the current page, and also supports any GET parameters that a clicked hash-link may have.

[Prinzhorn]

can't this be a one-liner? Like this

before

if(!/^#/.test(href)) {

after (not exactly, the logic is inverted for early exit etc)

if(link.hash && link.host === window.location.host) {
    //....
}

that's much cleaner and simpler than what you're doing with these string operations (I didn't even read through all of it)

[Prinzhorn]

oh well, never mind. it's not actually the same...

[charlesbaynham]

In a word, yup. That was silly of me, the DOM does it for me. See commits.

P.S. sorry for the silence, I've been away

[Prinzhorn]

But it's still not covering all cases I fear (neither did it before). Think of query parameters, which pathname doesn't cover. But stuff like ?page=home is still very common. So basically it's virtually impossible to detect if the link actually points to the exact same page. Any thoughts?

[charlesbaynham]

Hmmm true. I'd been intentionally filtering out query parameters but you're right, many sites do use them to show different pages. However, different query parameters ≠ different page in all cases.

I agree, it doesn't seem possible to check that it's the same page since we can't know whether or not the GET parameters will be used to completely change the page or not.

I would argue that this patch would still be an improvement over current behavior though since it would deal with URLs that don't rely on GET params. for serving pages.

[Prinzhorn]

How about putting this behind an option which is false by default? This way it won't break anything and people can enable it if they know what they're doing.

I'm also sorry for taking so long to answer.

[charlesbaynham]

No problem. I've added that in now, thoughts?

[Prinzhorn]

Please see my last line-comment and then then please combine all the commits (http://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit/)

[charlesbaynham]

Rebased and I've simplified the code almost like you asked, but I've had to leave a little regex in there: see the line comment.

[charlesbaynham]

What a mess. There you go