Minor feedback changes.

ampproject · Jul 23, 2018 · 34cc4ce · 34cc4ce
1 parent a866e2c
commit 34cc4ce
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 18 deletions.
diff --git a/extensions/amp-a4a/0.1/amp-ad-template.js b/extensions/amp-a4a/0.1/amp-ad-template.js
@@ -25,14 +25,14 @@ import {Services} from '../../../src/services';
 import {TemplateRenderer} from './template-renderer';
 import {TemplateValidator} from './template-validator';
 import {addParamToUrl} from '../../../src/url';
-import {camelCaseToDash, startsWith} from '../../../src/string';
 import {dev} from '../../../src/log';
+import {startsWith} from '../../../src/string';
 
 // These have no side-effects, and so may be reused between all instances.
 const validator = new TemplateValidator();
 const nameFrameRenderer = new NameFrameRenderer();
 
-export const DATA_REQUEST_VAR_PREFIX = 'request-var-';
+export const DATA_REQUEST_PARAM_PREFIX = 'requestParam';
 
 export class AmpAdTemplate extends AmpAdNetworkBase {
   /**
@@ -83,12 +83,17 @@ export class AmpAdTemplate extends AmpAdNetworkBase {
     // 'data-request-var-<field_name>=<val>`, and append &<field_name>=<val> to
     // the add request URL.
     Object.keys(this.element.dataset).forEach(dataField => {
-      const dataFieldInDash = camelCaseToDash(dataField);
-      if (startsWith(dataFieldInDash, DATA_REQUEST_VAR_PREFIX)) {
-        const requestVarName = dataFieldInDash.slice(
-            DATA_REQUEST_VAR_PREFIX.length, dataFieldInDash.length);
-        url = addParamToUrl(
-            url, requestVarName, this.element.dataset[dataField]);
+      if (startsWith(dataField, DATA_REQUEST_PARAM_PREFIX)) {
+        const requestParamName = dataField.slice(
+            DATA_REQUEST_PARAM_PREFIX.length, dataField.length);
+        if (requestParamName) {
+          // Set the first character to lower case, as reading it in camelCase
+          // will automatically put it into upper case.
+          const finalParamName = requestParamName.charAt(0).toLowerCase() +
+              requestParamName.slice(1);
+          url = addParamToUrl(
+              url, finalParamName, this.element.dataset[dataField]);
+        }
       }
     });
     this.getContext().adUrl = url;

diff --git a/extensions/amp-a4a/0.1/template-config.js b/extensions/amp-a4a/0.1/template-config.js
@@ -24,7 +24,7 @@ export let NetworkRegistryDef;
 
 /** @const {!NetworkRegistryDef} */
 export const NetworkRegistry = {
-  test: {
+  'custom-template': {
     requestUrl: '/adzerk/1',
   },
 };
diff --git a/extensions/amp-a4a/0.1/test/test-amp-ad-template.js b/extensions/amp-a4a/0.1/test/test-amp-ad-template.js
@@ -18,7 +18,7 @@ import {
   AMP_TEMPLATED_CREATIVE_HEADER_NAME,
   getAmpAdTemplateHelper,
 } from '../template-validator';
-import {AmpAdTemplate, DATA_REQUEST_VAR_PREFIX} from '../amp-ad-template';
+import {AmpAdTemplate} from '../amp-ad-template';
 import {AmpMustache} from '../../../amp-mustache/0.1/amp-mustache';
 import {NetworkRegistry} from '../template-config';
 import {data} from './testdata/valid_css_at_rules_amp.reserialized';
@@ -42,7 +42,7 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
     containerElement = document.createElement('div');
     containerElement.setAttribute('height', 50);
     containerElement.setAttribute('width', 320);
-    containerElement.setAttribute('type', 'test');
+    containerElement.setAttribute('type', 'custom-template');
     containerElement.signals = () => ({
       whenSignal: () => Promise.resolve(),
     });
@@ -172,15 +172,16 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
       impl.buildCallback();
       impl.getRequestUrl();
       expect(impl.getContext().adUrl).to
-          .equal(NetworkRegistry.test.requestUrl);
+          .equal(NetworkRegistry['custom-template'].requestUrl);
     });
     it('should expand url', () => {
       impl.buildCallback();
-      impl.element.setAttribute(`data-${DATA_REQUEST_VAR_PREFIX}bar`, '123');
-      impl.element.setAttribute(`data-${DATA_REQUEST_VAR_PREFIX}baz`, '456');
+      impl.element.setAttribute('data-request-param-bar', '123');
+      impl.element.setAttribute('data-request-param-baz', '456');
+      impl.element.setAttribute('data-request-param-camel-case', '789');
       impl.requestUrl_ = 'https://foo.com?sz=widthxheight';
       expect(impl.getRequestUrl()).to.equal(
-          'https://foo.com?sz=320x50&bar=123&baz=456');
+          'https://foo.com?sz=320x50&bar=123&baz=456&camelCase=789');
     });
   });
 
@@ -196,11 +197,11 @@ describes.realWin('TemplateRenderer', realWinConfig, env => {
           .throw('Invalid network type foo');
     });
     it('should throw if type does not have corresponding request url', () => {
-      const temp = NetworkRegistry.test.requestUrl;
-      delete NetworkRegistry.test.requestUrl;
+      const temp = NetworkRegistry['custom-template'].requestUrl;
+      delete NetworkRegistry['custom-template'].requestUrl;
       expect(() => new AmpAdTemplate(containerElement)).to
           .throw('Invalid network configuration: no request URL specified');
-      NetworkRegistry.test.requestUrl = temp;
+      NetworkRegistry['custom-template'].requestUrl = temp;
     });
   });
 });