Skip to content
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

routed links must descend from pushstate root #652

Merged
merged 1 commit into from
Jan 22, 2014
Merged

routed links must descend from pushstate root #652

merged 1 commit into from
Jan 22, 2014

Conversation

stevenvachon
Copy link
Contributor

If we have:

can.route.bindings.pushstate.root = "/bitovi/src/app/";

and a link like:

<a href="/bitovi/test/"></a>

When that link is clicked on, it will call ev.preventDefault() and do a pushState() instead of letting the link behave normally.


Additionally, if we have the same root listed above and:

<a href="/bitovi/src/app/test/"></a>
<!-- or -->
<a href="test/"></a>

can.route.deparam treats "/bitovi/src/app/" (the root) as part of the route when it shouldn't.

@justinbmeyer
Copy link
Contributor

What is the issue exactly? I'm not sure what your test code does because it's only logging things and I don't know where the fail assertion should be. Given your description:

If you have a pushstate root of "/app/" and you have a link within your app pointing to "/other/", it is currently be treated as a route in v2.0.4 because the host is the same.

Are you saying that if I have:

can.route.bindings.pushstate.root = "/bitovi/src/app/";

and a link like:

<a href="/bitovi/test/"></a>

When that link is clicked on, it will call ev.preventDefault() and do a pushState() instead of letting the link behave like normal?

If yes, that is certainly a bug.

@stevenvachon
Copy link
Contributor Author

Yes, exactly. I have a fix for it, though. And an additional fix will also avoid the issue I was having which led me to write #644. I've edited my original post above.

@stevenvachon
Copy link
Contributor Author

I am never using issue/PR numbers in a commit message ever again.

@justinbmeyer
Copy link
Contributor

Why? They are very useful.

@stevenvachon
Copy link
Contributor Author

When we do a git rebase -i and git push -f, Github will nicely remove any previous commits on the PR, but not if those commits have messages containing references to an issue/PR. Like the 3 above. I've emailed Github about it, though and perhaps they'll implement a new feature in the near future.

// window.routeTestReady gets called again
}
// if routed
timeout = setTimeout( function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs .stop() and .start() or .asyncTest for asynchronous tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scratch that. Looks good.

@daffl
Copy link
Contributor

daffl commented Jan 13, 2014

@justinbmeyer Can you sanity check this? Looks good to merge to me but I haven't worked with the pushstate implementation a lot.

andykant added a commit that referenced this pull request Jan 22, 2014
routed links must descend from pushstate root
@andykant andykant merged commit f5978c4 into canjs:master Jan 22, 2014
andykant added a commit that referenced this pull request Jan 22, 2014
andykant added a commit that referenced this pull request Jan 22, 2014
@justinbmeyer
Copy link
Contributor

@andykant where did you get on fixing this? This is breaking minor.

@stevenvachon stevenvachon deleted the patch-3 branch January 23, 2014 13:50
andykant added a commit that referenced this pull request Jan 23, 2014
Rewrote the pushstate #652 test for stability purposes
andykant added a commit that referenced this pull request Jan 24, 2014
The link wasn’t properly checking ev.isDefaultPrevented()
andykant added a commit that referenced this pull request Jan 24, 2014
Caused issues with YUI in Firefox
daffl added a commit that referenced this pull request Jan 29, 2014
Fixed the pushstate #652 test to ensure that it should pass in all libraries and phantomjs
@justinbmeyer
Copy link
Contributor

This test is still breaking for me in firefox.

justinbmeyer added a commit that referenced this pull request Mar 31, 2014
justinbmeyer added a commit that referenced this pull request Apr 1, 2014
daffl added a commit that referenced this pull request Apr 2, 2014
This reverts commit 90614ea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants