Added a very basic version of analytics for AMP. #982
Conversation
| * limitations under the License. | ||
| */ | ||
|
|
||
| amp-analytics { |
There was a problem hiding this comment.
This is too late. We have to move this definition to amp.css.
There was a problem hiding this comment.
done in a previous commit.
|
Please reference in the description:
|
| { | ||
| "on": "TAP", | ||
| "request": "default", | ||
| "selector": "" |
There was a problem hiding this comment.
What does empty selector do?
There was a problem hiding this comment.
undefined behavior for now. I removed it since the implementation for that part is not ready yet. Once we start working on it, we can define what empty selector does.
| */ | ||
| handleEvent_(trigger, event) { | ||
| const baseConfig = AnalyticsConfig[this.type_] || {}; | ||
| const host = this.config_['host'] || baseConfig['host']; |
There was a problem hiding this comment.
So, why are we still doing this if it was merged above?
There was a problem hiding this comment.
missed removing it. removed
|
Cool. I think we are pretty close. But one request: could you please respond to each comment we make? It makes it a lot easier to review. |
|
Good. So, two more actions:
Otherwise, LGTM. |
So far, the tag parses the inline config and uses one builtin type to listen for document.load events and sends a GET request when that happens. There is premitive support for request templates and platform variables. Things that are pending: - Support for more platform variables (future PR) - Better instrumentation class (future PR) - User specified variables (future PR) and more...
|
LGTM |
Added a very basic version of analytics for AMP.
|
👍 |
|
Woot. |
This is the first PR in a series of PRs to implement the spec outlined in #871.
In this commit, the tag parses the inline config and uses one builtin type to listen for
document.load events and sends a GET request when that happens. There is
primitive support for request templates and platform variables.
Things that are still to come: