Needed host check for pushstate.js #249

Merged
merged 3 commits into from Jan 23, 2013

Conversation

Projects
None yet
3 participants
@marshallswain
Member

marshallswain commented Jan 23, 2013

pushstate.js was only checking the pathname. Since HTML5 pushstate only works on the same hostname, other links should be allowed to work as normal. Links to other domains will now work. Also updated the variable name at lines 46 and 47 to be the pathname instead of the href, since that's what we are really working with at that point in the code.

marshallswain added some commits Jan 23, 2013

Need to check more than the pathname. Adding hostname check, since HT…
…ML5 pushstate only works on the same hostname. Links to other domains will now work.
For relative links in i.e., the host shows up as blank, so the last f…
…ix broke ie. A blank host really means the current host, so we can check to see if it's blank and assign the current host, then proceed.
@daffl

View changes

route/pushstate/pushstate.js
@@ -34,14 +34,17 @@ steal('can/util', 'can/route', function(can) {
_setup: function() {
// intercept routable links
can.$('body').on('click', 'a', function(e) {
- if(can.route.updateWith(this.pathname+this.search)) {
+ // Fix for ie showing blank host, but blank host means current host.
+ this.host == '' ? this.host = window.location.host : null;

This comment has been minimized.

@daffl

daffl Jan 23, 2013

Contributor

This looks a little weird. Isn't this just doing

if(!this.host) {
  this.host = window.location.host;
}

Besides that I'd merge this in because it makes sense. I was thinking about a good way for testing this but I don't think there is (even checking the window.location in an iframe wouldn't really tell us much).

@daffl

daffl Jan 23, 2013

Contributor

This looks a little weird. Isn't this just doing

if(!this.host) {
  this.host = window.location.host;
}

Besides that I'd merge this in because it makes sense. I was thinking about a good way for testing this but I don't think there is (even checking the window.location in an iframe wouldn't really tell us much).

This comment has been minimized.

@marshallswain

marshallswain Jan 23, 2013

Member

That does look better. I updated the pull request to match. Thanks.

Have a great day!

Marshall Thompson
Creative Ideal

On Wednesday, January 23, 2013 at 1:38 PM, David Luecke wrote:

In route/pushstate/pushstate.js:

@@ -34,14 +34,17 @@ steal('can/util', 'can/route', function(can) { > _setup: function() { > // intercept routable links > can.$('body').on('click', 'a', function(e) { > - if(can.route.updateWith(this.pathname+this.search)) { > + // Fix for ie showing blank host, but blank host means current host. > + this.host == '' ? this.host = window.location.host : null;
This looks a little weird. Isn't this just doing
if(!this.host) { this.host = window.location.host; }

Besides that I'd merge this in because it makes sense. I was thinking about a good way for testing this but I don't think there is (even checking the window.location in an iframe wouldn't really tell us anything).


Reply to this email directly or view it on GitHub (https://github.com/bitovi/canjs/pull/249/files#r2748417).

@marshallswain

marshallswain Jan 23, 2013

Member

That does look better. I updated the pull request to match. Thanks.

Have a great day!

Marshall Thompson
Creative Ideal

On Wednesday, January 23, 2013 at 1:38 PM, David Luecke wrote:

In route/pushstate/pushstate.js:

@@ -34,14 +34,17 @@ steal('can/util', 'can/route', function(can) { > _setup: function() { > // intercept routable links > can.$('body').on('click', 'a', function(e) { > - if(can.route.updateWith(this.pathname+this.search)) { > + // Fix for ie showing blank host, but blank host means current host. > + this.host == '' ? this.host = window.location.host : null;
This looks a little weird. Isn't this just doing
if(!this.host) { this.host = window.location.host; }

Besides that I'd merge this in because it makes sense. I was thinking about a good way for testing this but I don't think there is (even checking the window.location in an iframe wouldn't really tell us anything).


Reply to this email directly or view it on GitHub (https://github.com/bitovi/canjs/pull/249/files#r2748417).

daffl added a commit that referenced this pull request Jan 23, 2013

Merge pull request #249 from marshallswain/master
Needed host check for pushstate.js

@daffl daffl merged commit 5dfd70c into canjs:master Jan 23, 2013

1 check passed

default The Travis build passed
Details
@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 23, 2013

Contributor

Cool, thanks for the pull request. Hopefully we can make this plugin officially a part of 1.2. You definitely got it a step closer :)

Contributor

daffl commented Jan 23, 2013

Cool, thanks for the pull request. Hopefully we can make this plugin officially a part of 1.2. You definitely got it a step closer :)

@marshallswain

This comment has been minimized.

Show comment
Hide comment
@marshallswain

marshallswain Jan 24, 2013

Member

I know the documentation needs some work. What else needs to be done? It's working quite awesome for me in my projects, now, but I did have to prepend my routes in the controllers with a forward slash /. I played around with removing the forward slash, but it messed up the URL in the address bar whenever they were more than one level deep. I'll work on getting it to work without changing the controller routes.

Anything else you can think of?

Have a great day!

Marshall Thompson
Creative Ideal

On Wednesday, January 23, 2013 at 3:41 PM, David Luecke wrote:

Cool, thanks for the pull request. Hopefully we can make this plugin officially a part of 1.2. You definitely got it a step closer :)


Reply to this email directly or view it on GitHub (#249 (comment)).

Member

marshallswain commented Jan 24, 2013

I know the documentation needs some work. What else needs to be done? It's working quite awesome for me in my projects, now, but I did have to prepend my routes in the controllers with a forward slash /. I played around with removing the forward slash, but it messed up the URL in the address bar whenever they were more than one level deep. I'll work on getting it to work without changing the controller routes.

Anything else you can think of?

Have a great day!

Marshall Thompson
Creative Ideal

On Wednesday, January 23, 2013 at 3:41 PM, David Luecke wrote:

Cool, thanks for the pull request. Hopefully we can make this plugin officially a part of 1.2. You definitely got it a step closer :)


Reply to this email directly or view it on GitHub (#249 (comment)).

@talentedmrjones

This comment has been minimized.

Show comment
Hide comment
@talentedmrjones

talentedmrjones Jan 24, 2013

@daffl Where can I find some documentation on this pushstate code? I'm trying to use it but not getting very far. Reverse engineering isn't in the budget on this one :)

@daffl Where can I find some documentation on this pushstate code? I'm trying to use it but not getting very far. Reverse engineering isn't in the budget on this one :)

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 24, 2013

Contributor

@marshallswain: If you use it without any major problems, find a fix for the problem and would like to contribute documentation that would be greatly appreciated! Just create a pushstate.md in this folder with the docs.

@talentedmrjones: Unfortunately there isn't any yet. The only way at the moment is referring to the demo.html for usage.

Contributor

daffl commented Jan 24, 2013

@marshallswain: If you use it without any major problems, find a fix for the problem and would like to contribute documentation that would be greatly appreciated! Just create a pushstate.md in this folder with the docs.

@talentedmrjones: Unfortunately there isn't any yet. The only way at the moment is referring to the demo.html for usage.

moschel added a commit that referenced this pull request May 3, 2015

moschel added a commit that referenced this pull request May 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment