Added element-level overrides for variables for amp-analytics by rajkumarsrk · Pull Request #4072 · ampproject/amphtml

rajkumarsrk · 2016-07-14T19:11:53Z

Adding element level overrides for amp-analytics variable based on the discussion that we had
#1298 (comment).
Please review it and let me know your feedback

@avimehta @cramforce Could you suggest best place where I can update the documentation for this.
I was thinking to add it here
https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/amp-analytics.md#vars. But will wait for you inputs

googlebot · 2016-07-14T19:11:54Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

rudygalfi · 2016-07-14T19:31:05Z

This is specific to click trigger, so documentation within https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/amp-analytics.md#click-trigger-on-click would make the most sense.

I was wondering if it makes any sense at all to broaden this capability to other trigger types? I guess specifically visible now that it also has a selector attribute under visibilitySpec? cc @avimehta for feedback

https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/amp-analytics.md#vars

avimehta · 2016-07-14T21:27:02Z

@rudygalfi If you have seen requests for data-vars on visible event, we can add them there as well. I have not come across such requests yet.

@rajkumarsrk Will look at the PR soon. You should mention the presence of these variables in https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/amp-analytics.md#click-trigger-on-click but give details of how to use these variables in interaction section of https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/analytics-vars.md. I do something similar for scroll triggers.

rajkumarsrk · 2016-07-14T22:34:30Z

@avimehta @rudygalfi . Sure, I will update the documentation

rudygalfi · 2016-07-14T23:16:31Z

I don't believe I've seen requests. My proposal was purely a hypothetical need and can be ignored for v1 so we just focus on click.

avimehta · 2016-07-14T23:26:46Z

@rudygalfi actually, If you think it makes sense, let's do it in this PR. I am happy with whatever makes more sense to you.

rudygalfi · 2016-07-14T23:31:21Z

I think it makes sense. If it's not much additional work, I'd encourage adding it (and corresponding doc updates).

senthilp · 2016-07-15T00:16:58Z

@rudygalfi we will look into it and update the PR with analysis for other trigger types. If it is simple enough we can accommodate it in this PR itself.

rajkumarsrk · 2016-07-15T07:30:05Z

@avimehta Is the selector inside the "visible" eventType has to a sibling property to on eventType.

Currently we have it as

"triggers": {
  "defaultPageview": {
    "on": "visible",
    "request": "pageview",
    "visibilitySpec": {
      "selector": "#anim-id",
      "visiblePercentageMin": 20,
      "totalTimeMin": 500,
      "continuousTimeMin": 200
    }
  }
}

having the following would resemble the click trigger types

"triggers": {
  "defaultPageview": {
    "on": "visible",
     "selector": "#anim-id",
    "request": "pageview",
    "visibilitySpec": {
      "visiblePercentageMin": 20,
      "totalTimeMin": 500,
      "continuousTimeMin": 200
    }
  }
}

By this way it would resemble the click eventType format where on, selector, request would be siblings

"links": {
        "on": "click",
        "selector": "#test1",
        "request": "click",
        "vars": {
          "label": "clickChapter::clickLabel",
          "level2Click": "12",
          "type": "a"
        }
      }

The reason for this question is, I am planning to pass the parsed values of data-attributes from the element level as a sibling to selector on config from createVisibilityListener_ on instrumentation

avimehta · 2016-07-15T20:58:00Z

extensions/amp-analytics/0.1/instrumentation.js

Move this to top of the file with other contants?

avimehta · 2016-07-15T21:07:47Z

looks good so far.

@cramforce we talked about moving the selector one level up so that click and visible have the same place to specify selector. How do we go about doing a breaking change? Should we put a deprecation warning in the old format and start accepting the new format?

cramforce · 2016-07-15T22:32:21Z

It'll be tough to ever get rid of the old way (without bumping the amp-analytics version, which we would not for such a small thing), but we can definitely warn and keep supporting both.

rajkumarsrk · 2016-07-15T23:43:01Z

@avimehta Thanks for reviewing the changes. I will update this PR with review changes and adding support for the element level overrides for "visible" event.

rajkumarsrk · 2016-07-18T21:15:21Z

@avimehta & @cramforce Is it expected that the selector for visible event is always an Id ?

"triggers": {
  "defaultPageview": {
    "on": "visible",
    "request": "pageview",
    "visibilitySpec": {
      "selector": "#anim-id",
      "visiblePercentageMin": 20,
      "totalTimeMin": 500,
      "continuousTimeMin": 200
    }
  }
}

https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/0.1/visibility-impl.js#L241

rudygalfi · 2016-07-18T23:36:33Z

@rajkumarsrk I think it can also be a class name, e.g. .foo, but @avimehta should confirm.

rajkumarsrk · 2016-07-19T00:34:29Z

Got the answer for this from console "Visibility spec requires an id selector"

rajkumarsrk · 2016-07-19T19:06:07Z

@rudygalfi Thanks Rudy. Looks like we support only id only

avimehta · 2016-07-19T20:45:27Z

@rajkumarsrk yes. We might want to add support for other elements formats at a later point but for now it is just ids.

avimehta · 2016-07-20T02:18:24Z

extensions/amp-analytics/0.1/instrumentation.js

I think the latest example brings up an issue with this line. Vars have been case sensitive till now. If we lowercase the key, the casing gets lost. Can we avoid doing toLowerCase?

Update this to follow the same format of naming convention that is followed for other variables (camelCase) as in domain=${canonicalHost}&path=${canonicalPath}

data-vars-sub-title="Example sub title" will resolve to

{ "varsSubTitle": "Example sub title" }

Which in turn is resolved to

{ "subTitle": "Example sub title" }

avimehta · 2016-07-21T19:09:29Z

This looks pretty good overall. While we are fixing the final few pieces of feedback and adding documentation, do you mind also signing the CLA?

avimehta · 2016-07-21T19:10:19Z

extensions/amp-analytics/0.1/instrumentation.js

you should extract the variables and merge them with vars here.

For visibility.listenOnce we need both the visibilitySpec and the merged vars. Merging of vars is done on #L225

googlebot · 2016-07-28T00:15:54Z

CLAs look good, thanks!

rajkumarsrk · 2016-07-28T00:18:43Z

@avimehta @rudygalfi Thanks for reviewing the changes . I have update the code & documentations

jridgewell · 2016-07-28T17:51:16Z

extensions/amp-analytics/0.1/instrumentation.js

+            if (spec['selector']) {
+              const el = this.win_.document.getElementById(spec['selector']
+                .slice(1));
+              Object.assign(vars, this.extractAnalyticsVariables_(el));


Object.assign is not available in all environments! This will fail!

http://kangax.github.io/compat-table/es6/#test-Object_static_methods_Object.assign says that it is not supported IE11 and below, Safari 8 and below. But Babel and closure support it. What can we use here instead? Is a for-loop the best alternative if .assign is undefined?

Modified to for loop instead of Object.assign

avimehta · 2016-08-04T17:39:26Z

src/dom.js

-    const matches = attr.name.match(/^data-param-(.+)/);
+    const attrName = attr.name;
+    const matches = attrName.match(/^data-param-(.+)/) ||
+      attrName.match(/^data-vars-(.+)/);


I don't think this will work. This changes the behavior for existing pages/clients. I think we should add another parameter to the function that accepts a regex called opt_paramPattern. If the value is not provided, you can default to /^data-param-(.+)/. Then from instrumentation.js, pass in the value of the pattern.

@rajkumarsrk Can you address this comment?

Yup. Added opt_paramPattern and test case has been updated as well

avimehta · 2016-08-05T20:45:21Z

Just a few more comments and we are good to go! Thanks for your patience so far.

avimehta · 2016-08-08T19:02:02Z

src/dom.js

 * @param {!Element} element
 * @param {function(string):string} opt_computeParamNameFunc to compute the parameter
 *    name, get passed the camel-case parameter name.
+ * @param {string} Regex pattern to match data attributes.


please change type to string= and add the parameter name after the type.

@param {string=} opt_paramPattern Regex pattern to match data attributes.

avimehta · 2016-08-08T19:03:14Z

@jridgewell would you like to take another look?

@rajkumarsrk I just have one small comment.

rajkumarsrk · 2016-08-08T20:46:32Z

@avimehta 👍 Updated JS doc

avimehta · 2016-08-08T21:19:16Z

@jridgewell I'll merge this PR(to unblock other PRs). Feel free to comment here with more suggestions changes though. I'll address those in a separate PR.

rajkumarsrk · 2016-08-16T05:22:59Z

@avimehta Can you let me know when this feature will be available to consume

rudygalfi · 2016-08-16T13:31:07Z

@rajkumarsrk I believe this made it into last week's canary build. That means it should be in production by this Friday the 19th.

You can target's AMP's canary build if you want to try it out. Opt into the dev channel (first option) on this page: https://cdn.ampproject.org/experiments.html.

rajkumarsrk · 2016-08-17T04:13:44Z

Thanks Rudy

…ject#4072) * Adding element-level overrides for variables on amp-analytics - click, visibile * Updating with review feedback changes * Using dom.js for extracting data attributes * Updating amp-analytics variable overrides with review feedback * Fixing lint errors * Updating documentation for amp-analytics

rudygalfi assigned avimehta Jul 14, 2016

rudygalfi added the NEEDS REVIEW label Jul 14, 2016

avimehta reviewed Jul 15, 2016
View reviewed changes

avimehta reviewed Jul 20, 2016
View reviewed changes

avimehta reviewed Jul 21, 2016
View reviewed changes

Updating with review feedback changes

7686732

jridgewell reviewed Jul 28, 2016
View reviewed changes

rajkumarsrk added 2 commits August 1, 2016 20:23

Using dom.js for extracting data attributes

2720e3d

Merge branch 'master' of https://github.com/ampproject/amphtml

e23ae67

avimehta reviewed Aug 4, 2016
View reviewed changes

avimehta mentioned this pull request Aug 4, 2016

Added ability to specify tag names in selectors. #4368

Merged

rajkumarsrk added 3 commits August 6, 2016 15:42

Updating amp-analytics variable overrides with review feedback

ee2413b

Merge branch 'master' of https://github.com/ampproject/amphtml

3fe4dfe

Fixing lint errors

46e9f19

avimehta reviewed Aug 8, 2016
View reviewed changes

avimehta added LGTM and removed NEEDS REVIEW labels Aug 8, 2016

avimehta mentioned this pull request Aug 8, 2016

Added support for a new trigger on=hidden. #4265

Merged

Updating documentation for amp-analytics

dff8aa2

avimehta merged commit 5e2eb4a into ampproject:master Aug 8, 2016

georgecrawford mentioned this pull request Aug 16, 2016

Analytics Financial-Times/google-amp#124

Merged

Conversation

rajkumarsrk commented Jul 14, 2016

Uh oh!

googlebot commented Jul 14, 2016

Uh oh!

rudygalfi commented Jul 14, 2016

Uh oh!

avimehta commented Jul 14, 2016

Uh oh!

rajkumarsrk commented Jul 14, 2016

Uh oh!

rudygalfi commented Jul 14, 2016

Uh oh!

avimehta commented Jul 14, 2016

Uh oh!

rudygalfi commented Jul 14, 2016

Uh oh!

senthilp commented Jul 15, 2016

Uh oh!

rajkumarsrk commented Jul 15, 2016

Uh oh!

avimehta Jul 15, 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!

avimehta commented Jul 15, 2016

Uh oh!

cramforce commented Jul 15, 2016

Uh oh!

rajkumarsrk commented Jul 15, 2016

Uh oh!

rajkumarsrk commented Jul 18, 2016

Uh oh!

rudygalfi commented Jul 18, 2016

Uh oh!

rajkumarsrk commented Jul 19, 2016

Uh oh!

rajkumarsrk commented Jul 19, 2016

Uh oh!

avimehta commented Jul 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avimehta commented Jul 21, 2016

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!

googlebot commented Jul 28, 2016

Uh oh!

rajkumarsrk commented Jul 28, 2016

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!

avimehta commented Aug 5, 2016

Uh oh!

avimehta Aug 8, 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!

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

Uh oh!

Uh oh!

avimehta Jul 15, 2016 •

edited

Loading

avimehta Aug 8, 2016 •

edited

Loading

avimehta commented Aug 8, 2016 •

edited

Loading