-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Added a very basic version of analytics for AMP. #982
Conversation
* limitations under the License. | ||
*/ | ||
|
||
amp-analytics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too late. We have to move this definition to amp.css.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done in a previous commit.
Please reference in the description:
|
{ | ||
"on": "TAP", | ||
"request": "default", | ||
"selector": "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does empty selector do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @param {string} formatString The request string to be expanded | ||
* @param {RegExp} expression The regular expression used to detect request | ||
* templates | ||
* @param {Object.<string, string> requests Map of requests to lookup template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dots in types and '!'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
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: