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 AT Internet #1672

Closed
wants to merge 97 commits into from

Conversation

@BenDz
Copy link
Contributor

commented Jan 29, 2016

This request adds support for AT Internet solution as a vendor.

Tagging example:

<amp-analytics id="atinternet">
    <script type="application/json">
        {
            "vars": {
                "site": "123456",
                "log": "logs",
                "domain": ".xiti.com",
                "title": "pageChapter::pageTitle",
                "level2": "10"
            },
            "triggers": {
                "defaultPageview": {
                    "on": "visible",
                    "request": "pageview"
                },
                "links": {
                    "on": "click",
                    "selector": "a",
                    "request": "click",
                    "vars": {
                        "label": "clickChapter::clickLabel",
                        "level2Click": "12",
                        "type": "a"
                    }
                }
            }
        }
    </script>
</amp-analytics>

Our own documentation for integration is being written.

BenDz added 2 commits Jan 29, 2016
@googlebot

This comment has been minimized.

Copy link

commented Jan 29, 2016

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.
@cramforce

This comment has been minimized.

Copy link
Member

commented Jan 29, 2016

Thanks for the pull request! Handing over to @avimehta and @rudygalfi

@@ -79,5 +79,22 @@ export const ANALYTICS_CONFIG = {
},
'optout': '_gaUserPrefs.ioo'
}

'atinternet': {

This comment has been minimized.

Copy link
@rudygalfi

rudygalfi Jan 29, 2016

Contributor

I'd suggest sequencing this above googleanalytics above to keep things alphabetically organized.

@@ -79,5 +79,22 @@ export const ANALYTICS_CONFIG = {
},
'optout': '_gaUserPrefs.ioo'
}

'atinternet': {
'transport': {'beacon': false, 'xhrpost': false},

This comment has been minimized.

Copy link
@rudygalfi

rudygalfi Jan 29, 2016

Contributor

Would it be best to be explicit here and include 'image': true in this list?

cc @avimehta

BenDz added 2 commits Feb 3, 2016
@avimehta

This comment has been minimized.

Copy link
Contributor

commented on extensions/amp-analytics/0.1/vendors.js in 27397b5 Feb 3, 2016

What is x0 at the end of the line? Of it is color depth, we now support it. Feel free to add it.

@avimehta

This comment has been minimized.

Copy link
Contributor

commented on extensions/amp-analytics/0.1/vendors.js in 27397b5 Feb 3, 2016

What are the vars level2, level2Click? Are the supposed to be filled in by the publisher? Are they required params? Of so, please document them.

@avimehta

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

Please sign the cla and reply to this thread with "I signed it!".

Also, please add an example of how to use the new type in examples/analytics.amp.html

Once that is done, Also, please squash the commits into a single commit and rebase to head as there are a few changes to the files you are submitting...

Thanks for the PR!

Jordan M Adler and others added 5 commits Feb 4, 2016
Jordan M Adler
@BenDz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2016

Signature is in progress, it will be for the corporation AT Internet.

@avimehta

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2016

Ben: will look at the commit later today. Thanks For the update!

Sebastien F and others added 6 commits Feb 5, 2016
Sebastien F Sebastien F
Also starts mandating AMP CORS protocol for credentialed requests to amp-list.
Verified that the guardian is currently not including credentials in their requests.
Adding Smart AdServer support
Enable retrieving amp-analytics remote config with credentials.
Access: require CORS/AMP authorization
Gregable and others added 17 commits Feb 9, 2016
- Avoid using the return value of addPositionTo (which is
  Token, not ErrorToken)
- Assert that the elements returned by querySelectorAll are
  HTMLElement instances.
Create new UrlSpec usable by AttrSpec for allowed protocols in href/src
attributes.
Validate URLs are not missing
Validate URLs are valid (via goog.Uri.parse)
Validate protocols are allowed for certain tags
Add colon to Entities list for htmlparser.
Fix lookupEntity_ in htmlparser to actually look up the entity by name.

anchor's href: http, https, mailto, sms, whatsapp, viber
img's src: http, https
amp-audio > source's src: http, https
amp-video > source's src: https
audio > source's src: http, https
video > source's src: https
amp-img's src: data, http, https
amp-anim's src: data, http, https
amp-video's src: https
amp-ad's src: https
amp-embed's src: https
amp-youtube's src: https
amp-twitter's src: https
amp-instagram's src: https
amp-iframe's src: https
amp-pixel's src: https
amp-audio's src: http, https
amp-list's src: https
amp-install-serviceworker's src: https
sync with the published spec.
update gulp to shut up the warnings
Validation roll-up
Loading indicator for amp-embed
Also filters out changes that happen in the same milli seconds.
Deletes the `amp-iframe` intersection tests because they are super bad and I already filed #1822 to rewrite them.
This is better than local, but ideally this would point at the right tag.
Reference sourcemaps from GitHub.
Throttle intersection observer posts to 10 per second.
Cache URL parsing results.
@BenDz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

Hi @rudygalfi - I was OoO yesterday, will check it today and squash the commits to a single one.
Thanks!

@googlebot

This comment has been minimized.

Copy link

commented Feb 10, 2016

CLAs look good, thanks!

BenDz added 3 commits Feb 5, 2016
Example fixed
@googlebot

This comment has been minimized.

Copy link

commented Feb 10, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@BenDz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

Hi, I created a new clean PR for this integration. Please check #1908.
Thanks!

@rudygalfi

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

Closing to move over to #1908

@rudygalfi rudygalfi closed this Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.