routed links must descend from pushstate root #652

Merged
merged 1 commit into from Jan 22, 2014

Conversation

Projects
None yet
4 participants
@stevenvachon
Contributor

stevenvachon commented Jan 8, 2014

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 9, 2014

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.

Contributor

justinbmeyer commented Jan 9, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 9, 2014

Contributor

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.

Contributor

stevenvachon commented Jan 9, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 12, 2014

Contributor

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

Contributor

stevenvachon commented Jan 12, 2014

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 12, 2014

Contributor

Why? They are very useful.

Contributor

justinbmeyer commented Jan 12, 2014

Why? They are very useful.

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 12, 2014

Contributor

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.

Contributor

stevenvachon commented Jan 12, 2014

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() {

This comment has been minimized.

@daffl

daffl Jan 13, 2014

Contributor

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

@daffl

daffl Jan 13, 2014

Contributor

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

This comment has been minimized.

@daffl

daffl Jan 13, 2014

Contributor

Scratch that. Looks good.

@daffl

daffl Jan 13, 2014

Contributor

Scratch that. Looks good.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 13, 2014

Contributor

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

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

Merge pull request #652 from stevenvachon/patch-3
routed links must descend from pushstate root

@andykant andykant merged commit f5978c4 into canjs:master Jan 22, 2014

1 check passed

default The Travis CI build passed
Details

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 23, 2014

Contributor

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

Contributor

justinbmeyer commented Jan 23, 2014

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

@stevenvachon stevenvachon deleted the stevenvachon:patch-3 branch Jan 23, 2014

andykant added a commit that referenced this pull request Jan 23, 2014

Merge pull request #699 from bitovi/new_pushstate_test
Rewrote the pushstate #652 test for stability purposes

andykant added a commit that referenced this pull request Jan 24, 2014

More fixes for the #652 push state test
The link wasn’t properly checking ev.isDefaultPrevented()

andykant added a commit that referenced this pull request Jan 24, 2014

Tweaked how link clicks were triggered in #652 test
Caused issues with YUI in Firefox

daffl added a commit that referenced this pull request Jan 29, 2014

Merge pull request #702 from bitovi/2.0.5_test_fixes
Fixed the pushstate #652 test to ensure that it should pass in all libraries and phantomjs
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 31, 2014

Contributor

This test is still breaking for me in firefox.

Contributor

justinbmeyer commented Mar 31, 2014

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

Revert "cleans #652 test"
This reverts commit 90614ea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment