Skip to content

Fix a bug causing resources with relative paths to load the wrong URL #371

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

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

imaustink
Copy link
Contributor

@imaustink imaustink commented Jul 5, 2017

We highjack clicks on <a> tags and use Ajax to load the content in. If we don't change the URL with pushState before we load the content into the DOM, all relative paths will be based off the previous page's URL. This causing the issue Justin reported in slack: https://bitovi.slack.com/archives/C09R50SAE/p1499289090158230

@@ -218,6 +218,9 @@ function navigate(href) {
return xhr;
},
success: function(content) {

window.history.pushState(null, null, href);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chasenlehara I think this is where we want it.

Copy link
Member

@chasenlehara chasenlehara left a comment

Choose a reason for hiding this comment

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

This PR also fixes a pretty nasty analytics issue: we were always sending Google Analytics the page the user last visited instead of the page they just navigated to. 😢

static/canjs.js Outdated
@@ -25,6 +25,10 @@ var $articleContainer,
hasShownSearch;

(function() {

// Google Analytics
ga('send', 'pageview', window.location.pathname);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chasenlehara don't we already send the initial page load here:

ga('send', 'pageview');

Copy link
Member

Choose a reason for hiding this comment

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

You are right, thanks for pointing that out! I’ve removed my commit.

@imaustink imaustink merged commit 6a80af4 into master Jul 6, 2017
@imaustink imaustink deleted the fix-relative-paths branch July 6, 2017 18:46
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.

2 participants