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

Changing can.Control.route selectors to match pushstate.js #612

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@marshallswain
Member

marshallswain commented Dec 16, 2013

When using can.Control.route with pushstate.js the events never fire because the selectors don't match those in pushstate.js. This is because before a route is added to can.route.routes it is stripped of any preceding forward slash. I've added the same check on line 9. This causes routes to match and controller events fire. This shouldn't affect routes using #! since those routes won't start with a forward slash.

Please pardon the whitespace stripping. I've only added lines 9-11.

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Dec 17, 2013

Member

Added lots more documentation for using pushstate.js with can.Control.route

Member

marshallswain commented Dec 17, 2013

Added lots more documentation for using pushstate.js with can.Control.route

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 18, 2013

Contributor

Thanks. I will check this out tomorrow.

Contributor

justinbmeyer commented Dec 18, 2013

Thanks. I will check this out tomorrow.

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Dec 18, 2013

Member

Is it possible to listen to two routes in on one function, or in other words two route selectors in the same function name... something like the example routes, below.

"/routeone route, /routetwo route": function(data){
  // /routeone and /routetwo are both handled here.
}

"/routeone, /routetwo route": function(data){
  // /routeone and /routetwo are both handled here.
}

Obviously these don't work or I wouldn't be asking. Maybe the feature is there and I just don't understand the syntax? I know this works for other event listeners to separate them by commas. If it is the case, this fix isn't enough to handle multiple routes, but will at least handle the first.

Also, I've not tried testing this out with hash-change routing. I HAVE tested push state routes with a custom build of can that includes all options in the download builder. I also tried using only the below scripts and was able to find a "fix" in can.Control.route.

<script src="/can/can.jquery.js"></script>
<script src="/can/can.route.pushstate.js"></script>
<script src="/can/can.map.delegate.js"></script>
Member

marshallswain commented Dec 18, 2013

Is it possible to listen to two routes in on one function, or in other words two route selectors in the same function name... something like the example routes, below.

"/routeone route, /routetwo route": function(data){
  // /routeone and /routetwo are both handled here.
}

"/routeone, /routetwo route": function(data){
  // /routeone and /routetwo are both handled here.
}

Obviously these don't work or I wouldn't be asking. Maybe the feature is there and I just don't understand the syntax? I know this works for other event listeners to separate them by commas. If it is the case, this fix isn't enough to handle multiple routes, but will at least handle the first.

Also, I've not tried testing this out with hash-change routing. I HAVE tested push state routes with a custom build of can that includes all options in the download builder. I also tried using only the below scripts and was able to find a "fix" in can.Control.route.

<script src="/can/can.jquery.js"></script>
<script src="/can/can.route.pushstate.js"></script>
<script src="/can/can.map.delegate.js"></script>
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 18, 2013

Contributor

@marshallswain You are not able to do that with can.Control. However, it's best to separate distinct ideas into their own issue so not to confuse / lose things.

Btw, I'm of the opinion that can/control/route should be removed from CanJS. You should be listening to changes in can.route like you do a can.Map. I've talked about this a lot on the forums. If you want to know more, please bring it up there.

Contributor

justinbmeyer commented Dec 18, 2013

@marshallswain You are not able to do that with can.Control. However, it's best to separate distinct ideas into their own issue so not to confuse / lose things.

Btw, I'm of the opinion that can/control/route should be removed from CanJS. You should be listening to changes in can.route like you do a can.Map. I've talked about this a lot on the forums. If you want to know more, please bring it up there.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 19, 2013

Contributor

@marshallswain I'm not sure exactly what this fixes as a test is not included. Anyway you can share something that I can verify breaks before and is fixed after the code changes?

Contributor

justinbmeyer commented Dec 19, 2013

@marshallswain I'm not sure exactly what this fixes as a test is not included. Anyway you can share something that I can verify breaks before and is fixed after the code changes?

### Using `can.route.pushstate.js` with the `can.Control.route` plugin
The `can.Control.route` plugin is a great way to simplify your code. Not only will it bind the event listeners for routes, but it also automagically prevents clicks on matching links from reloading the entire page.

This comment has been minimized.

@justinbmeyer

justinbmeyer Dec 20, 2013

Contributor

Thanks a ton for the docs! I'm going to have to change some of this text b/c using can.Control.route isn't really "recommended".

@justinbmeyer

justinbmeyer Dec 20, 2013

Contributor

Thanks a ton for the docs! I'm going to have to change some of this text b/c using can.Control.route isn't really "recommended".

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 20, 2013

Contributor

@marshallswain Why are you putting slashes in-front of your routes? Would everything work if you didn't have a slash?

Also, I think you should be checking if the route starts with can.route.bindings.pushstate.root not only /.

I'm going to move this to 2.0.5 as it needs some more discussion.

Thanks a ton for your help!

Contributor

justinbmeyer commented Dec 20, 2013

@marshallswain Why are you putting slashes in-front of your routes? Would everything work if you didn't have a slash?

Also, I think you should be checking if the route starts with can.route.bindings.pushstate.root not only /.

I'm going to move this to 2.0.5 as it needs some more discussion.

Thanks a ton for your help!

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Dec 20, 2013

Member

@justinbmeyer I didn't even realize I could take the slashes out until just now. When pushstate.js first showed up on the forums I played with it and at the time it required the beginning slashes. I had tried it without slashes, but it still wasn't working for me. Now I realize that it was because of the folder structure headache that I "discovered" and put at the bottom of the documentation.

I just tried the routes without the beginning slashes. They work both with and without the change to can.Control.route.js. With the edit I made, they work either with or without slashes.

Definitely need to include a check for the pushstate.root instead of /.

Member

marshallswain commented Dec 20, 2013

@justinbmeyer I didn't even realize I could take the slashes out until just now. When pushstate.js first showed up on the forums I played with it and at the time it required the beginning slashes. I had tried it without slashes, but it still wasn't working for me. Now I realize that it was because of the folder structure headache that I "discovered" and put at the bottom of the documentation.

I just tried the routes without the beginning slashes. They work both with and without the change to can.Control.route.js. With the edit I made, they work either with or without slashes.

Definitely need to include a check for the pushstate.root instead of /.

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 23, 2014

Contributor

I included your changes and added a test in #700.

Contributor

andykant commented Jan 23, 2014

I included your changes and added a test in #700.

@andykant andykant closed this Jan 23, 2014

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

Merge pull request #700 from bitovi/can-control-route_selectors
Changing can.Control.route to match pushstate (#612)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment