Implement amp-call-tracking extension#7493
Conversation
8b0011c to
58b2f7c
Compare
|
|
||
| tags: { # amp-call-tracking | ||
| html_format: AMP | ||
| html_format: AMP4ADS |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should restrict this one for amp4ads.
There was a problem hiding this comment.
AMP4ADS removed from validator rules.
| supported_layouts: NODISPLAY | ||
| supported_layouts: RESPONSIVE | ||
| } | ||
| child_tags: { |
There was a problem hiding this comment.
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.
| <script async src="https://cdn.ampproject.org/v0.js"></script> | ||
| </head> | ||
| <body> | ||
| <!-- Example of valid amp-call-tracking usage --> |
There was a problem hiding this comment.
This is a nit, but ideally each would start with either Valid: or Invalid: so here I'd use:
| tag_name: "AMP-CALL-TRACKING" | ||
| requires: "amp-call-tracking extension .js script" | ||
| attrs: { | ||
| name: "config" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
58b2f7c to
ddaf887
Compare
ddaf887 to
53521b6
Compare
| return layout === Layout.FIXED || | ||
| layout === Layout.FIXED_HEIGHT || | ||
| layout === Layout.RESPONSIVE || | ||
| layout === Layout.CONTAINER || |
There was a problem hiding this comment.
Container? If we omit this, we can use isLayoutSizeDefined instead.
|
|
||
| /** @override */ | ||
| buildCallback() { | ||
| this.hyperlink_ = this.getRealChildren()[0]; |
There was a problem hiding this comment.
Can we just query for the <a> directly? Something like querySelector('a[href^="tel:"]')?
There was a problem hiding this comment.
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
hrefattribute to contain atel: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, |
There was a problem hiding this comment.
Length check is unnecessary.
|
|
||
| /** @override */ | ||
| layoutCallback() { | ||
| return urlReplacementsForDoc(this.getAmpDoc()).expandAsync(assertHttpsUrl( |
There was a problem hiding this comment.
Can we do the assert in the #buildCallback for early errors?
|
PTAL. |
jridgewell
left a comment
There was a problem hiding this 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]; |
There was a problem hiding this comment.
Continuing #7493 (comment):
In that case, can we just use this.element.firstElementChild?
| @@ -0,0 +1,68 @@ | |||
| <!--- | |||
There was a problem hiding this comment.
It'd be good to cross reference the design doc and this PR because this is such a new component.
|
@jridgewell for merge |
* 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
* 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
|
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! |
Fixes #7340 and #5276.
Design.