Send form-submit success/error analytics events by mkhatib · Pull Request #5868 · ampproject/amphtml

mkhatib · 2016-10-28T00:12:08Z

mkhatib · 2016-10-28T17:41:28Z

/cc @avimehta @rudygalfi let me know if this looks good - event naming and if example should be updated - I don't know 100% if I am using the analytics config correct.

jridgewell · 2016-10-28T19:29:44Z

extensions/amp-form/0.1/amp-form.js

+   * @param {?Object<string, string>=} opt_vars A map of vars and their values.
+   * @private
+   */
+  analyticsEvent_(eventType, opt_vars) {


Isn't it this.win?

No, amp-form is not a base-element.

avimehta

mostly LGTM. Feel free to merge if others approve before me and you have addressed non-nit comments.

avimehta · 2016-10-28T21:22:29Z

examples/forms.amp.html

+    <script type="application/json">
+        {
+            "requests": {
+                "test-ping": "https://my-analytics.com/ping?title=${title}&acct=${account}",


nit: This URL will show an errror in network tab. Slight preference for a real URL like: https://www.ampproject.org/static/img/logo-blue.svg.

avimehta · 2016-10-28T21:23:23Z

examples/forms.amp.html

+                "test-ping": "https://my-analytics.com/ping?title=${title}&acct=${account}",
+                "event": "https://my-analytics.com/ping?eid=${eventId}&elab=${eventLabel}"
+            },
+            "vars": {


nit: Nothing is wrong with this exampe but this is not necessary to showcase analytics abilities in forms. Maybe remove? Also remove ${account} from the request?

avimehta · 2016-10-28T21:24:02Z

examples/forms.amp.html

+                "account": "12345"
+            },
+            "triggers": {
+                "page-view": {


nit: this can be removed as this isn't specific to forms.

avimehta · 2016-10-28T21:25:53Z

src/analytics.js

+
+
+/**
+ * Helper method to trigger analytics event if analytics is available.


if amp-analytics is available?

avimehta · 2016-10-28T21:26:33Z

extensions/amp-form/0.1/amp-form.js

        requireAmpResponseSourceOrigin: true,
      }).then(response => {
        this.actions_.trigger(this.form_, 'submit-success', null);
+        this.analyticsEvent_('amp-form-submit-success');


These need to be documented. Please see how access and slides document this and use the same convention?

Yeah the plan was to document these once this change is in prod otherwise we'll get lots of issues opened that this isn't working if we update docs right away. I'll send a follow up PR later.

file a bug and add a todo here.

avimehta · 2016-10-28T21:27:32Z

extensions/amp-analytics/0.1/instrumentation.js


  /**
   * @param {!AnalyticsEventType|string} type The type of event.
-   * @param {!Object<string, string>=} opt_vars A map of vars and their values.


Confused. How does ! and = work together? How is it different from ? and = being used together?

Afaik, the = allows undefined and the ? allows null as values.

= means the parameter can be omitted:

fn({}); fn();

so we don't really need ? here then?

Using ! instead.

avimehta · 2016-10-28T22:06:20Z

src/analytics.js

                window, 'amp-analytics-instrumentation', 'amp-analytics')));
 };
+
+


nit: extra empty line?

camelburrito · 2016-11-01T23:14:47Z

src/analytics.js

+ * @returns {!Promise<boolean>} Whether the event was sent or not.
+ */
+export function triggerAnalyticsEvent(window, eventType, opt_vars) {
+  return analyticsForOrNull(window).then(analytics => {


why do we have to return anything here, seems like it is not being used anywhere?

camelburrito · 2016-11-01T23:16:35Z

src/analytics.js

+export function triggerAnalyticsEvent(window, eventType, opt_vars) {
+  return analyticsForOrNull(window).then(analytics => {
+    if (!analytics) {
+      return false;


i guess we should also be doing some kind of logging here, so that we know if we are triggering before analytics is available.

camelburrito · 2016-11-01T23:16:54Z

src/analytics.js

+      return false;
+    }
+    analytics.triggerEvent(eventType, opt_vars);
+    return true;


i don't think this is needed either.

camelburrito · 2016-11-01T23:17:25Z

extensions/amp-carousel/0.1/slidescroll.js

 import {Animation} from '../../../src/animation';
 import {BaseSlides} from './base-slides';
-import {analyticsForOrNull} from '../../../src/analytics';
+import {triggerAnalyticsEvent} from '../../../src/analytics';


you must also make amp-access use this.

camelburrito

Please fix the comments above!

mkhatib · 2016-11-02T21:48:35Z

@camelburrito fixed comments PTAL 👀

camelburrito · 2016-11-02T22:07:26Z

extensions/amp-form/0.1/amp-form.js

        requireAmpResponseSourceOrigin: true,
      }).then(response => {
        this.actions_.trigger(this.form_, 'submit-success', null);
+        this.analyticsEvent_('amp-form-submit-success');


file a bug and add a todo here.

camelburrito · 2016-11-02T22:12:36Z

src/analytics.js

+ * @param {string} eventType
+ * @param {!Object<string, string>=} opt_vars A map of vars and their values.
+ */
+export function triggerAnalyticsEvent(window, eventType, opt_vars) {


tests for this?

mkhatib · 2016-11-05T00:24:59Z

@camelburrito 👀 PTAL

mkhatib added the NEEDS REVIEW label Oct 28, 2016

mkhatib assigned cramforce, avimehta and rudygalfi Oct 28, 2016

jridgewell reviewed Oct 28, 2016

View reviewed changes

avimehta reviewed Oct 28, 2016

View reviewed changes

avimehta approved these changes Oct 28, 2016

View reviewed changes

mkhatib assigned camelburrito and unassigned cramforce Nov 1, 2016

camelburrito reviewed Nov 1, 2016

View reviewed changes

camelburrito suggested changes Nov 1, 2016

View reviewed changes

mkhatib added 5 commits November 2, 2016 14:47

Send form-submit success/error analytics events.

0819e6d

fix types

1b72b26

address comments

cf230fd

fix comments

cbf7cec

update access

acc106f

mkhatib force-pushed the submit-analytics branch from b7b1619 to acc106f Compare November 2, 2016 21:48

camelburrito suggested changes Nov 2, 2016

View reviewed changes

mkhatib mentioned this pull request Nov 4, 2016

Document amp-form analytics events #6032

Closed

add test and todo

198efc2

camelburrito approved these changes Nov 7, 2016

View reviewed changes

mkhatib removed the NEEDS REVIEW label Nov 7, 2016

mkhatib merged commit 926b444 into ampproject:master Nov 7, 2016

mkhatib deleted the submit-analytics branch November 7, 2016 21:41

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Send form-submit success/error analytics events (ampproject#5868)

0c32769

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Send form-submit success/error analytics events (ampproject#5868)

44f7604



		/**
		* Helper method to trigger analytics event if analytics is available.

		window, 'amp-analytics-instrumentation', 'amp-analytics')));
		};

Conversation

mkhatib commented Oct 28, 2016

Uh oh!

mkhatib commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camelburrito left a comment

Choose a reason for hiding this comment

Uh oh!

mkhatib commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

avimehta Oct 28, 2016 •

edited

Loading

avimehta Oct 28, 2016 •

edited

Loading