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.

Sign up for GitHub

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

Jump to bottom

Fix amp-analytics related bugs. #1547

Merged
merged 1 commit into from
Jan 25, 2016
Merged

Fix amp-analytics related bugs. #1547

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions extensions/amp-analytics/0.1/vendors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,30 @@ export const ANALYTICS_CONFIG = {
'scrollHeight': 'SCROLL_HEIGHT',
'screenWidth': 'SCREEN_WIDTH',
'screenHeight': 'SCREEN_HEIGHT'
// Vars that GA can use: documentEncoding, userLanguage
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this comment? Can this be made into a new issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets add it to #1503? I'll remove this line from here once the issue is updated or a new one is created.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done: Added to #1503

}
// TODO(btownsend, #871): Add a generic hit format to make custom analytics
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this into an issue? And clarify it some more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were thiking of providing a generic request format with all the basic properties added to it. That way all publishers don't have to keep defining their own hit format. We have not seen any requests about it so maybe I can remove the comment and just wait for someone to request it?

// easier.
},

'googleanalytics': {
'vars': {
'eventValue': "0",
'documentLocation': 'AMPDOC_URL'
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: why not dl=${ampdocUrl} below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do that, people will have to override the ampdocUrl property to set a different value for dl. That looks and feel counter-intuitive. SInce this is a common thing many people do, I opted for this solution instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 makes sense, thanks

},
'requests': {
'host': 'https://www.google-analytics.com',
'basePrefix': 'v=1&_v=a0&aip=true&_s=${hitCount}&dl=${canonicalUrl}&' +
'basePrefix': 'v=1&_v=a0&aip=true&_s=${hitCount}&dr=${documentReferrer}' +
'dt=${title}&sr=${screenWidth}x${screenHeight}&_utmht=${timestamp}&' +
'jid=&cid=${clientId(_ga)}&tid=${account}',
'baseSuffix': '&z=${random}',
'jid=&cid=${clientId(_ga)}&tid=${account}&dl=${documentLocation}',
'baseSuffix': '&a=${pageViewId}&z=${random}',
'pageview': '${host}/r/collect?${basePrefix}&t=pageview&' +
'_r=1${baseSuffix}',
'event': '${host}/collect?${basePrefix}&t=event&' +
'ec=${eventCategory}&ea=${eventAction}&el=${eventLabel}&' +
'ev=${eventValue}${baseSuffix}',
'social': '${host}/collect?${basePrefix}&t=social&' +
'sa=${socialAction}&sn=${socialNetwork}&st=${socialActionTarget}' +
'sa=${socialAction}&sn=${socialNetwork}&st=${socialTarget}' +
'${baseSuffix}'
},
'optout': '_gaUserPrefs.ioo'
Expand Down