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

Implement amp-call-tracking extension #7493

Merged
merged 6 commits into from Feb 14, 2017

Conversation

Projects
None yet
5 participants
@alanorozco
Copy link
Member

commented Feb 11, 2017

Fixes #7340 and #5276.

Design.

@alanorozco alanorozco force-pushed the alanorozco:amp-call-tracking branch from 8b0011c to 58b2f7c Feb 11, 2017


tags: { # amp-call-tracking
html_format: AMP
html_format: AMP4ADS

This comment has been minimized.

Copy link
@honeybadgerdontcare

honeybadgerdontcare Feb 11, 2017

Contributor

We limit a lot of extensions for amp4ads, are you sure this one is going to go in? It's not listed in amp4ads whitelist. Same for actual tag below.

This comment has been minimized.

Copy link
@jasti

jasti Feb 11, 2017

Collaborator

We should restrict this one for amp4ads.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 11, 2017

Author Member

AMP4ADS removed from validator rules.

supported_layouts: NODISPLAY
supported_layouts: RESPONSIVE
}
child_tags: {

This comment has been minimized.

Copy link
@honeybadgerdontcare

honeybadgerdontcare Feb 11, 2017

Contributor

We attempt to keep these listed in the order they're listed in validator.proto. So spec_url and amp_layout should come after child_tags.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 11, 2017

Author Member

Done.

<script async src="https://cdn.ampproject.org/v0.js"></script>
</head>
<body>
<!-- Example of valid amp-call-tracking usage -->

This comment has been minimized.

Copy link
@honeybadgerdontcare

honeybadgerdontcare Feb 11, 2017

Contributor

This is a nit, but ideally each would start with either Valid: or Invalid: so here I'd use:

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 11, 2017

Author Member

Done.

tag_name: "AMP-CALL-TRACKING"
requires: "amp-call-tracking extension .js script"
attrs: {
name: "config"

This comment has been minimized.

Copy link
@honeybadgerdontcare

honeybadgerdontcare Feb 11, 2017

Contributor

I see the config attribute listed here, but not the phoneNumber attribute which is mentioned as required in the docs below. So either this needs to be updated or the docs need to be updated.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 11, 2017

Author Member

phoneNumber is a field that must be present in the CORS response, it's not part of the HTML schema.

It's a little confusing in the source for the documentation, it looks better when the Markdown file is actually formatted :)

@alanorozco alanorozco force-pushed the alanorozco:amp-call-tracking branch from 58b2f7c to ddaf887 Feb 11, 2017

@alanorozco alanorozco force-pushed the alanorozco:amp-call-tracking branch from ddaf887 to 53521b6 Feb 11, 2017

return layout === Layout.FIXED ||
layout === Layout.FIXED_HEIGHT ||
layout === Layout.RESPONSIVE ||
layout === Layout.CONTAINER ||

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 11, 2017

Member

Container? If we omit this, we can use isLayoutSizeDefined instead.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

Cool.


/** @override */
buildCallback() {
this.hyperlink_ = this.getRealChildren()[0];

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 11, 2017

Member

Can we just query for the <a> directly? Something like querySelector('a[href^="tel:"]')?

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

That is better for clarity, true. But:

  • Querying for an <a> element specifically is not needed since the validator ensures that the child node will be an anchor tag.

  • Checking for the href attribute to contain a tel: prefix is a little overzealous. If the client sets the default phone number incorrectly that's up to them.

I don't feel very strongly about this either way, but I thought these are important aspects to account for.

this.element.getAttribute('config'), this.element))
.then(url => fetch_(this.win, url))
.then(data => {
user().assert(data.phoneNumber && data.phoneNumber.length,

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 11, 2017

Member

Length check is unnecessary.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

True.


/** @override */
layoutCallback() {
return urlReplacementsForDoc(this.getAmpDoc()).expandAsync(assertHttpsUrl(

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 11, 2017

Member

Can we do the assert in the #buildCallback for early errors?

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

Done.

alanorozco added some commits Feb 13, 2017

@alanorozco alanorozco modified the milestone: Sprint H2 Feb Feb 13, 2017

@alanorozco

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

PTAL.

@jridgewell
Copy link
Member

left a comment

Leaving to @jasti for final approval. Just ping back when I should merge.

this.configUrl_ = assertHttpsUrl(
this.element.getAttribute('config'), this.element);

this.hyperlink_ = this.getRealChildren()[0];

This comment has been minimized.

Copy link
@jridgewell

jridgewell Feb 13, 2017

Member

Continuing #7493 (comment):

In that case, can we just use this.element.firstElementChild?

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

Done.

@@ -0,0 +1,68 @@
<!---

This comment has been minimized.

Copy link
@jasti

jasti Feb 13, 2017

Collaborator

It'd be good to cross reference the design doc and this PR because this is such a new component.

This comment has been minimized.

Copy link
@alanorozco

alanorozco Feb 13, 2017

Author Member

Done.

@jasti

jasti approved these changes Feb 13, 2017

@alanorozco

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2017

@jridgewell for merge

@jridgewell jridgewell merged commit 3c4ae7a into ampproject:master Feb 14, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alanorozco alanorozco deleted the alanorozco:amp-call-tracking branch Feb 14, 2017

torch2424 added a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017

Implement amp-call-tracking extension (ampproject#7493)
* Implement amp-call-tracking extension

* Use isLayoutSizeDefined

* Assert valid URL early

* Remove unnecessary length check

* Use firstElementChild to select anchor element

* Cross-reference design doc and PR for amp-call-tracking element

mrjoro added a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Implement amp-call-tracking extension (ampproject#7493)
* Implement amp-call-tracking extension

* Use isLayoutSizeDefined

* Assert valid URL early

* Remove unnecessary length check

* Use firstElementChild to select anchor element

* Cross-reference design doc and PR for amp-call-tracking element
@taf2

This comment has been minimized.

Copy link

commented Aug 3, 2017

Missing from the design document is the format expected for the phoneNumber in the config response. I'm assuming E.164 but that should be clearly called out. leading + or not... Also no mention of how to match up live session data from the config request with an analytics request in the analytics tag... a complete solution would combine both... possibly the call tracking tag itself should include two endpoints. 1 for the configuration as you have with this PR but also another for sending tracking pixels. this way session information can be shared... or an example of how to share between the config request and another analytics tag request... thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.