added a new template for adobe_analytics using iframePing #3035
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g.
|
|
I signed the CLA under Adobe's company agreement |
|
/to @avimehta for review |
| @@ -458,6 +459,16 @@ export const ANALYTICS_CONFIG = { | |||
| }, | |||
| }, | |||
|
|
|||
| 'adobeanalytics_iframePing': { | |||
There was a problem hiding this comment.
Lets rename to not include iframePing. How about view?
Would like to enable us to not encode the implementation in the name.
There was a problem hiding this comment.
No problem making this change but am a little unsure of your ask. Are you asking for "adobeanalytics_iframeView", or "adobeanalytics_view"?
There was a problem hiding this comment.
Something without the word iframe in it that makes sense to you.
cory-work
force-pushed
the
master
branch
2 times, most recently
from
33ceef9
to
9c63d08
Compare
| @@ -458,6 +459,15 @@ export const ANALYTICS_CONFIG = { | |||
| }, | |||
| }, | |||
|
|
|||
| 'adobeanalytics_nativeConfig': { | |||
There was a problem hiding this comment.
Can you add an example in analytics.amp.html and some documentation to amp-analytics.md?
There was a problem hiding this comment.
We have doc and example on our site. Would you be okay with a link to those docs and samples?
There was a problem hiding this comment.
yes, that would be perfect!
There was a problem hiding this comment.
I've added an example in analytics.amp.html. We already have a link to our documentation in amp-analytics.md.
There was a problem hiding this comment.
I don't see updates to amp-analytics.md. Maybe that file got missed?
There was a problem hiding this comment.
@avimehta We already have an entry in amp-analytics.md :)
| <amp-analytics type=adobeanalytics> | ||
| <script type="application/json"> | ||
| { | ||
| "requests": { |
There was a problem hiding this comment.
this looks good but just wanted to point out that we can do this using extraUrlParams. I just sent out a PR for the piece that was missing. #3168
There was a problem hiding this comment.
Do you mean the items I have listed in the vars below? Yeah I know that extraUrlParams can be used here. In fact I'm a big fan of the extraUrlParams. Historically, variable expansion has been a little dodgy in the extraUrlParams; is that what your PR addresses?
Made "image" the only transport used for adobeanalytics Added Adobe Analytics example to examples/analytics.amp.html Added Adobe Analytics native config example to examples/analytics.amp.html
|
CLA needs verification. the PR looks good to me otherwise. |
|
CLA-wise is fine to merge after cory's comment. @avimehta Feel free to merge when your last comment is addressed. |
|
I've signed the CLA under the Adobe company umbrella. I don't see any other issues. Was there another issue you wanted me to take a look at? |
|
@cramforce I don't have any other comments. Unfortunately, the merge button is not active for me (because of CLA i think) so you'll have to merge. |
|
@cramforce when will this make it into the wild? |
|
Likely next Thursday. |
|
And some time tomorrow you can try it in dev channel. |
|
Awesome. Thanks! |
Solves #3034