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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

marshallswain
Copy link
Member

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
Copy link
Member Author

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

@justinbmeyer
Copy link
Contributor

Thanks. I will check this out tomorrow.

@marshallswain
Copy link
Member Author

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
Copy link
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.

@justinbmeyer
Copy link
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?

### 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
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!

@marshallswain
Copy link
Member Author

@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
Copy link
Contributor

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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants