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.

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

Adds support for comScore #1608

Merged
merged 1 commit into from
Feb 1, 2016
Merged

Adds support for comScore #1608

merged 1 commit into from
Feb 1, 2016

Conversation

acorretti-comscore
Copy link
Contributor

This PR adds support for comScore UDM pageview analytics.

Var c2 must be defined containing customer id. A brief note about this requirement has been added to amp-analytics.md documentation.

@googlebot
Copy link

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 it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@dvoytenko
Copy link
Contributor

@avimehta for the initial review

@avimehta
Copy link
Contributor

there is a lint error:

extensions/amp-analytics/0.1/vendors.js
80:66 error Infix operators must be spaced space-infix-ops

apart from that and the optional comment, LGTM.

@avimehta
Copy link
Contributor

Also, if you guys would like, you can add an example of comscore analytics to examples/analytics.amp.html

@rudygalfi
Copy link
Contributor

cc @jmadler

@acorretti-comscore
Copy link
Contributor Author

When can this request be expected to be merged into AMP's codebase?

@cramforce
Copy link
Member

When all review comments are addressed.

Please upload a new version of the change that passes our travis tests and also squash commits. Thanks!

@acorretti-comscore
Copy link
Contributor Author

There you go with the travis error.
All comments should now be addressed. Thanks!

# The first commit's message is:

Adding type='comscore' to <amp-analytics>.

# The 2nd commit message will be skipped:

#	Adding comScore example pageview tracking
@acorretti-comscore
Copy link
Contributor Author

Somehow the lint task is still failing, yet the error message doesn't seem to suggest there's any problem in our code.

We'd appreciate some guidance if it needs to be solved before merging as we'd like to update our customers from the amp-pixel tag.
Thank you.

@cramforce
Copy link
Member

Sorry, about that. We just confirmed elsewhere that the lint error is happening on master. If other comments are addressed we can merge.

@avimehta
Copy link
Contributor

avimehta commented Feb 1, 2016

lgtm

cramforce added a commit that referenced this pull request Feb 1, 2016
Adds support for comScore
@cramforce cramforce merged commit 3ffd791 into ampproject:master Feb 1, 2016
@iefserge
Copy link
Contributor

iefserge commented Feb 3, 2016

thank you. I'm trying to setup this in analytics, but getting AmpAnalytics comscore No triggers were found in the config. No analytics data will be sent.. It works on dev channel though.

@rudygalfi
Copy link
Contributor

@iefserge I'm guessing it's because this hasn't been built into an AMP release yet.

@avimehta would that error correlate with AMP not knowing about the comscore label?

@avimehta
Copy link
Contributor

avimehta commented Feb 3, 2016

It might. It is most likely the release issue. I'll fix the error and be more descriptive.

@ghost
Copy link

ghost commented Apr 21, 2016

I have a question , for example , if I have a photo gallery for each slide and generate a new URL and sent to comScore an generate a new pageview. But in amp how I can do that? any ideas?

@rudygalfi
Copy link
Contributor

How is the photo gallery implemented in AMP?

The visible trigger will only fire once per pageview. The click trigger could listen to the arrows of, for example, the amp-carousel. We are also working on an element-level visibility trigger in #1297 that may be applicable.

Last thing I'd note regarding analytics integrations with vendors like comScore is that you should make sure the data being sent to the vendor complies with how the vendor expects it to be sent.

@ghost
Copy link

ghost commented Apr 21, 2016

Yes i use amp-carousel with the next format

`

... ` actually i use the trigger click over the selector div.amp-carousel-button but that generate the same url, i need to build a new url for example ${base}-1 for the first photo, ${base}-2 for the next, etc

it's that possible?

@rudygalfi
Copy link
Contributor

There is presently no way to update URL based on position in the amp-carousel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants