Snowplow v2 #26059
Snowplow v2 #26059
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
|
Local
Let me know if anything needs fixing! |
|
@max-tgam I think we might need you to comment |
|
@googlebot I consent. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@zhouyx Tagging you as the reviewer on the old PR (not sure who would be reviewer) :). |
colmsnowplow
force-pushed
the
snowplow-v2
branch
from
a2d32a1
to
dae186e
Compare
|
Apologies for force-pushing after requesting review - I noticed that I forgot to cherry-pick and squash in a commit. Have given it another once-over and it's definitely ready for review now (and merge if no changes required). :) |
There was a problem hiding this comment.
Looks good, please note I only reviewed the structure of the config. Please make sure that you've tested the outgoing request thoroughly.
We are in the middle of migrating all vendor config from js file to json file. The idea is that the config doesn't need to be compiled into the amp-analytics.js, instead AMP can load them lazily. To proceed, please also check in a json version of the snowplow_v2.json file. If you have the local developing environment set up, you can use the provided gulp task to generate that file by running gulp generate-vendor-jsons.
Note we can help you generate the json file for you in a separate PR, but it would be difficult to sync if we need to rollback the change here. So still highly recommend that you submit the json file along with this PR. Thank you!
Please refer to #26050 for more information on the migration process.
| }, | ||
| 'cookies': { | ||
| 'enabled': true, | ||
| 'cookieMaxAge': 1800, |
There was a problem hiding this comment.
To confirm, the cookeMaxAge unit is in second. It is the desired behavior to persist cookie for 30 mins?
There was a problem hiding this comment.
Ah no actually I want default value of 1 yr. Thanks for catching, commit on the way to fix.
There was a problem hiding this comment.
Got it, in that case, you don't need to specify the value. The default value is 365 days.
examples/analytics-vendors.amp.html
Outdated
| @@ -1687,11 +1777,13 @@ | |||
| </div> | |||
|
|
|||
| <div class="logo"></div> | |||
| <h1 id="top">AMP Analytics</h1> | |||
There was a problem hiding this comment.
My bad! Fixing.
| <span id="test1" class="box" data-vars-title="Example request with element level overrides"> | ||
| Click here to generate an event | ||
| </span> | ||
| <p> | ||
| Example of <a id="ext" data-vars-target-url="../" data-vars-element-id="home" data-vars-element-content="Internal Link" href="../">Internal Link</a>. |
There was a problem hiding this comment.
Cool, thanks for adding this to test linker feature
| @@ -0,0 +1,89 @@ | |||
| /** | |||
| * Copyright 2018 The AMP HTML Authors. All Rights Reserved. | |||
|
Thanks, done - note that I force added the JSON file due to the gitignore. |
Thank you for responding so quickly : ) #26053 had just been merged this morning : ) |
|
@colmsnowplow PR merged. Thank you for the integration! |
This PR replaces #24261 (Existing changes in there and authorship preserved, refactor added and commit history cleaned up).
Proposed changes for a PR to ampproject/amphtml.