-
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
Add new amp-analytics vendor: Keen #19360
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
Hello! Thanks for making an intent to implement issue and opening a PR to add yourself! 😄 I noticed that you are still committing to the PR. Please mention me (@) whenever this is ready, and we can do a review. Thank you! |
Oh wait just realized you were just fixing tests and things, will review this now! |
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.
LGTM Waiting on @zhouyx response to get this merged in.
Also, went ahead and tested this, and everything looks fine 😄
@@ -1101,7 +1147,7 @@ | |||
<amp-analytics type="teaanalytics" id="teaanalytics"> | |||
<script type="application/json"> | |||
{ | |||
"vars": { | |||
"vars": { |
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 fixing these line endings for us! Didn't catch those last time.
'viewportHeight': '${viewportHeight}', | ||
'viewportWidth': '${viewportWidth}', | ||
}, | ||
'transport': { |
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.
@zhouyx Could you remind me about how the transport works ( especially what to check for image ? )
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.
@torch2424 in our case transport called "image" wouldn't work, because we need to receive Post Body content in our API.
Some people want to track "clicks", so we need to rely on the BeaconAPI (to run requests in background).
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.
@adamkasprowicz What about browsers that don't support BeaconAPI, do you want to fallback to using xhrpost?
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.
@zhouyx yes, xhrpost also works for us - I changed the config value to True.
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.
Thank you. LGTM
Everyone LGTM'd Merging 😄 Also, you probably want to also add yourself to our docs site for supported analytics vendors: https://github.com/ampproject/docs/tree/master/content/docs/analytics Thanks for the PR! |
* add analytics vendor keen.io * fix analytics lint for vendor keen * fix comment for Keen tracking example code * fix tests for keen analytics vendor * vendor keen analytics custom request test * fix custom test unhandled variable * fix test variable ending * use xhr transport if beacon API is not available
related issue #19359