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

Adding Deep.BI analytics to AMP #25757

Merged
merged 2 commits into from Dec 3, 2019
Merged

Conversation

pkalinowski
Copy link
Contributor

Follow up of #25128
(fixing CLA - "cannot verify e-mail" issue)

Intent-to-implement ticket: #25070
@micajuine-ho @zhouyx - Thank you for your input. I hope this time it will get smoother :)

Follow up of ampproject#25128
(fixing CLA - "cannot verify e-mail" issue)
Intent-to-implement ticket: ampproject#25070
@calebcordry
Copy link
Member

Adding @ mentions as reviewers.

@calebcordry calebcordry removed their request for review November 25, 2019 14:28
Comment on lines 114 to 115
'ampUserLocation': 'AMP_USER_LOCATION',
'ampUserLocationPoll': 'AMP_USER_LOCATION_POLL',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these getting added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have taken an older version of the file.
Fixed! Thanks!

Delete old, improper lines. As in:
ampproject#25757 (comment)
@pkalinowski
Copy link
Contributor Author

Thank you very much @micajuine-ho!!!

Hello @zhouyx, I hope we'll be going with our analytics for AMP soon now :)

@pkalinowski
Copy link
Contributor Author

Hello @calebcordry, @micajuine-ho, @zhouyx ,

As far as I can see @zhouyx is on her vacation or otherwise is not available at the moment. Could you please @calebcordry and @micajuine-ho revise the review request to @zhouyx?
I will be the most grateful for any help in pushing my little contribution forwad :]
Thank you!

Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pkalinowski and @micajuine-ho

LGTM. As @micajuine-ho pointed out in the old thread. Please let users know that requestOrigin will only accepts an absolute URL, and only the origin of the URL will be used. https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/amp-analytics.md#request-origin for more information.

Thanks

@zhouyx zhouyx merged commit 9506628 into ampproject:master Dec 3, 2019
@zhouyx
Copy link
Contributor

zhouyx commented Dec 3, 2019

PR merged. Thanks

micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* Adding Deep.BI analytics to AMP

Follow up of ampproject#25128
(fixing CLA - "cannot verify e-mail" issue)
Intent-to-implement ticket: ampproject#25070

* Update vendors.js

Delete old, improper lines. As in:
ampproject#25757 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants