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 element-level overrides for variables for amp-analytics #4072

Merged
merged 8 commits into from Aug 8, 2016

Conversation

Projects
None yet
7 participants
@rajkumarsrk
Contributor

rajkumarsrk commented Jul 14, 2016

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

This comment has been minimized.

googlebot commented Jul 14, 2016

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

This comment has been minimized.

Contributor

rudygalfi commented Jul 14, 2016

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

This comment has been minimized.

Collaborator

avimehta commented Jul 14, 2016

@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

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 14, 2016

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

@rudygalfi

This comment has been minimized.

Contributor

rudygalfi commented Jul 14, 2016

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

This comment has been minimized.

Collaborator

avimehta commented Jul 14, 2016

@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

This comment has been minimized.

Contributor

rudygalfi commented Jul 14, 2016

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

@senthilp

This comment has been minimized.

senthilp commented Jul 15, 2016

@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

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 15, 2016

@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

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
*/
extractAnalyticsVariables_(el) {
const dataSet = el.dataset;
const vars = 'vars';

This comment has been minimized.

@avimehta

avimehta Jul 15, 2016

Collaborator

Move this to top of the file with other contants?

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
if (selector === '*' || this.matchesSelector_(e.target, selector)) {
listener(new AnalyticsEvent(AnalyticsEventType.CLICK));
if (selector === '*' || this.matchesSelector_(el, selector)) {
// See https://github.com/ampproject/amphtml/issues/1298

This comment has been minimized.

@avimehta

avimehta Jul 15, 2016

Collaborator

you don't need this comment. We can find the details using blame history of this line.

Same below.

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
* @param {!Function} listener
* @param {string} selector
* @private
*/
createSelectiveListener_(listener, selector) {
return e => {
let el = e.target;
let dataSet = null;

This comment has been minimized.

@avimehta

avimehta Jul 15, 2016

Collaborator

maybe call this analyticsVariables or just variables?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 15, 2016

Contributor

Yup. analyticsVariables will best fit for this

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Jul 15, 2016

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

This comment has been minimized.

Member

cramforce commented Jul 15, 2016

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

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 15, 2016

@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

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 18, 2016

@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

This comment has been minimized.

Contributor

rudygalfi commented Jul 18, 2016

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

@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 19, 2016

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

@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 19, 2016

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

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Jul 19, 2016

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

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
const analyticsVariables = {};
for (const dataKey in dataSet) {
if (dataKey.indexOf(vars) === 0) {
const variableName = dataKey.toLowerCase().replace(vars,'');

This comment has been minimized.

@avimehta

avimehta Jul 20, 2016

Collaborator

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?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

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

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
@@ -28,6 +28,7 @@ const DEFAULT_MAX_TIMER_LENGTH_SECONDS_ = 7200;
const SCROLL_PRECISION_PERCENT = 5;
const VAR_H_SCROLL_BOUNDARY = 'horizontalScrollBoundary';
const VAR_V_SCROLL_BOUNDARY = 'verticalScrollBoundary';
const vars = 'vars';

This comment has been minimized.

@avimehta

avimehta Jul 20, 2016

Collaborator

please make the name caps.

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

Sure

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

Update this to name caps

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
if (config['visibilitySpec']) {
const spec = config['visibilitySpec'];
const selector = config['selector'] || (spec && spec['selector']);
const analyticsVariables = config[vars] || {};

This comment has been minimized.

@avimehta

avimehta Jul 20, 2016

Collaborator

i don't understand this line. why are we reading from config[vars]? is some value expected to be there?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

By this we can have all the config level vars merged with element level variable overrides. This is similar to the tracking for 'click' event

This comment has been minimized.

@avimehta

avimehta Jul 21, 2016

Collaborator

but we don't do that for click event. In click event, the config level variables are merged with dataset variables in amp-analytics.js.

I think what you want to do is merge the analyticsVariables var with vars before Line 231.

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
if (!isVisibilitySpecValid(config)) {
return;
}
const el = this.win_.document.getElementById(selector

This comment has been minimized.

@avimehta

avimehta Jul 20, 2016

Collaborator

this and following three lines can be moved inside the callback passed into the listenOnce call below(#L231).

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

I had the same feeling that we are doing the DOM reference in 2 place, one in instrumentation.js and other on visibility-impl.js. But when you check the implementation done for the 'click' event we have the element level overrides extracted on the createSelectiveListener_. And we have access to extractAnalyticsVariables_ inside instrumentation.js and not on visibility-impl.js. Let me know your thoughts on this

This comment has been minimized.

@avimehta

avimehta Jul 21, 2016

Collaborator

What I mean is that you can do this just before Line 231. The difference is that in one case, the variables are extracted during config registeration while in the other case, the variables are extracted when the trigger fires.

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
@@ -212,14 +213,21 @@ export class InstrumentationService {
* @private
*/
createVisibilityListener_(callback, config) {
if (config['visibilitySpec']) {
const spec = config['visibilitySpec'];
const selector = config['selector'] || (spec && spec['selector']);

This comment has been minimized.

@avimehta

avimehta Jul 20, 2016

Collaborator

is this and following variable needed outside the if block?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 20, 2016

Contributor

Moved these variables inside the if block

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Jul 21, 2016

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

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
this.runOrSchedule_(() => {
visibilityFor(this.win_).then(visibility => {
visibility.listenOnce(config['visibilitySpec'], vars => {
visibility.listenOnce(config, vars => {
callback(new AnalyticsEvent(AnalyticsEventType.VISIBLE, vars));

This comment has been minimized.

@avimehta

avimehta Jul 21, 2016

Collaborator

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

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 21, 2016

Contributor

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

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Jul 22, 2016

Contributor

Done

@avimehta

View changes

extensions/amp-analytics/0.1/instrumentation.js Outdated
@@ -276,29 +284,51 @@ export class InstrumentationService {
}
/**
* @param {!Element} el
* @private
* @return {Object}

This comment has been minimized.

@avimehta

avimehta Jul 21, 2016

Collaborator

!Object<string, string>?

@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Jul 28, 2016

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

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

This comment has been minimized.

@jridgewell

jridgewell Jul 28, 2016

Member

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

This comment has been minimized.

@avimehta

avimehta Jul 28, 2016

Collaborator

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?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Aug 2, 2016

Contributor

Modified to for loop instead of Object.assign

for (const dataKey in dataSet) {
if (dataKey.indexOf(VARS) === 0) {
let variableName = dataKey.replace(VARS, '');
variableName = variableName[0].toLowerCase() + variableName.slice(1);

This comment has been minimized.

@jridgewell

jridgewell Jul 28, 2016

Member

Why would this be uppercase? It's data-vars-variable right?

This comment has been minimized.

@avimehta

avimehta Jul 28, 2016

Collaborator

element.dataSet convers everything that follows data into camel case. since 'v' in vars is the first letter, 'v' in variables is capital.

This is the transformation this code is doing:

data-vars-variable-name -> varsVariableName -> VariableName -> variableName

This comment has been minimized.

@jridgewell

jridgewell Jul 28, 2016

Member

Ahh, 👍 .

* @private
* @return {!Object<string, string>}
*/
extractAnalyticsVariables_(el) {

This comment has been minimized.

@jridgewell

jridgewell Jul 28, 2016

Member

Can this be merged with dom.getDataParamsFromAttributes?

This comment has been minimized.

@avimehta

avimehta Jul 28, 2016

Collaborator

Yes, this looks like a good suggestion. Let's do it.

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Aug 2, 2016

Contributor

Update to use dom.getDataParamsFromAttributes

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

This comment has been minimized.

@avimehta

avimehta Aug 4, 2016

Collaborator

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.

This comment has been minimized.

@jridgewell

jridgewell Aug 4, 2016

Member

Agreed.

This comment has been minimized.

@avimehta

avimehta Aug 5, 2016

Collaborator

@rajkumarsrk Can you address this comment?

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Aug 7, 2016

Contributor

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

src/dom.js Outdated
const matches = attr.name.match(/^data-param-(.+)/);
const attrName = attr.name;
const matches = attrName.match(/^data-param-(.+)/) ||
attrName.match(/^data-vars-(.+)/);
if (matches) {
const param = dashToCamelCase(matches[1]);

This comment has been minimized.

@avimehta

avimehta Aug 4, 2016

Collaborator

@jridgewell any idea why we can't/don't use dataSet here to get the attributes? Seems like that would be a little faster as it does not iterate over all the attributes and doesn't have to do camelCasing here.

This comment has been minimized.

@jridgewell

jridgewell Aug 4, 2016

Member

That'd be fine with me.

This comment has been minimized.

@avimehta

This comment has been minimized.

@rajkumarsrk

rajkumarsrk Aug 7, 2016

Contributor

@avimehta Updated to use dataset

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Aug 5, 2016

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

src/dom.js Outdated
@@ -400,24 +399,25 @@ export function childElementsByTag(parent, tagName) {
* @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.

This comment has been minimized.

@avimehta

avimehta Aug 8, 2016

Collaborator

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

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

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

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Aug 8, 2016

@jridgewell would you like to take another look?

@rajkumarsrk I just have one small comment.

@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Aug 8, 2016

@avimehta 👍 Updated JS doc

@avimehta

This comment has been minimized.

Collaborator

avimehta commented Aug 8, 2016

@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.

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

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Aug 16, 2016

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

@rudygalfi

This comment has been minimized.

Contributor

rudygalfi commented Aug 16, 2016

@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.

@georgecrawford georgecrawford referenced this pull request Aug 16, 2016

Merged

Analytics #124

@rajkumarsrk

This comment has been minimized.

Contributor

rajkumarsrk commented Aug 17, 2016

Thanks Rudy

ariangibson added a commit to Mixpo/amphtml that referenced this pull request Sep 7, 2016

Added element-level overrides for variables for amp-analytics (amppro…
…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

mityaha added a commit to ooyala/amphtml that referenced this pull request Nov 30, 2016

Added element-level overrides for variables for amp-analytics (amppro…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment