-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix amp-analytics related bugs. #1547
Conversation
'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}&' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity: Would you not want to send the referrer somewhere here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that. Added document referrer and also added a comment about vars that would be nice to have...
Tuned PR title and description a bit. |
ptal. |
Have a more customizable documentLocation for GA. This way, if a person wants to send both canonicalUrl and ampdocUrl in a hit, they can.
} | ||
// TODO(btownsend, #871): Add a generic hit format to make custom analytics | ||
// easier. | ||
}, | ||
|
||
'googleanalytics': { | ||
'vars': { | ||
'eventValue': "0", | ||
'documentLocation': 'AMPDOC_URL' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 makes sense, thanks
LGTM |
Fix amp-analytics related bugs.
Fixes #1494
Fixes #1491
cc: @btownsend