Skip to content

Commit

Permalink
pr comments
Browse files Browse the repository at this point in the history
  • Loading branch information
kmh287 committed Mar 27, 2017
1 parent 6b44719 commit d3a0f42
Show file tree
Hide file tree
Showing 9 changed files with 26 additions and 16 deletions.
2 changes: 2 additions & 0 deletions build-system/tasks/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ function compile(entryModuleFilenames, outputDir,
'extensions/amp-analytics/**/*.js',
// For amp-bind in the web worker (ww.js).
'extensions/amp-bind/**/*.js',
// Needed to access form impl from other extensions
'extensions/amp-form/**/*.js',
'src/*.js',
'src/!(inabox)*/**/*.js',
'!third_party/babel/custom-babel-helpers.js',
Expand Down
9 changes: 4 additions & 5 deletions extensions/amp-bind/0.1/bind-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,14 +380,13 @@ export class Bind {
const tagName = element.tagName;

let dynamicElements = [];
if (typeof element.getDynamicElements === 'function') {
dynamicElements = element.getDynamicElements();
if (typeof element.getDynamicElementContainers === 'function') {
dynamicElements = element.getDynamicElementContainers();
} else if (element.tagName === 'FORM') {
// FORM is not an amp element, so it doesn't have the getter directly.
const form = formOrNullForElement(element);
if (form) {
dynamicElements = form.getDynamicElements();
}
dev().assert(form, 'could not find form implementation');
dynamicElements = form.getDynamicElementContainers();
}
dynamicElements.forEach(elementToObserve => {
this.mutationObserver_.observe(elementToObserve, {childList: true});
Expand Down
5 changes: 4 additions & 1 deletion extensions/amp-bind/0.1/test/test-bind-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,15 @@
*/

import * as sinon from 'sinon';
import {AmpForm} from '../../../amp-form/0.1/amp-form';
import {Bind} from '../bind-impl';
import {BindExpression} from '../bind-expression';
import {BindValidator} from '../bind-validator';
import {chunkInstanceForTesting} from '../../../../src/chunk';
import {installTimerService} from '../../../../src/service/timer-impl';
import {toArray} from '../../../../src/types';
import {toggleExperiment} from '../../../../src/experiments';
import {user} from '../../../../src/log';
import {installTimerService} from '../../../../src/service/timer-impl';

describes.realWin('Bind', {
amp: {
Expand Down Expand Up @@ -173,6 +174,8 @@ describes.realWin('Bind', {
const dynamicTag = doc.createElement('div');
dynamicTag.setAttribute('submit-success', null);
form.appendChild(dynamicTag);
// Wrap form in amp-form implementation so bind can access it
new AmpForm(form);
return onBindReady().then(() => {
expect(bind.boundElements_.length).to.equal(0);
const elementWithBinding = createElementWithBinding('[onePlusOne]="1+1"');
Expand Down
10 changes: 7 additions & 3 deletions extensions/amp-form/0.1/amp-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -594,14 +594,18 @@ export class AmpForm {
* @return {Array<!Element>}
* @public
*/
getDynamicElements() {
getDynamicElementContainers() {
const dynamicElements = [];
const successDiv =
this.form_./*OK*/querySelector(`[${FormState_.SUBMIT_SUCCESS}]`);
const errorDiv =
this.form_./*OK*/querySelector(`[${FormState_.SUBMIT_ERROR}]`);
successDiv && dynamicElements.push(successDiv);
errorDiv && dynamicElements.push(errorDiv);
if (successDiv) {
dynamicElements.push(successDiv);
}
if (errorDiv) {
dynamicElements.push(errorDiv);
}
return dynamicElements;
}

Expand Down
2 changes: 1 addition & 1 deletion extensions/amp-live-list/0.1/amp-live-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -841,7 +841,7 @@ export class AmpLiveList extends AMP.BaseElement {
}

/** @override */
getDynamicElements() {
getDynamicElementContainers() {
return this.itemsSlot_ ? [this.itemsSlot_] : [];
}

Expand Down
4 changes: 2 additions & 2 deletions src/base-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -900,10 +900,10 @@ export class BaseElement {
* element owns that could have children added or removed dynamically.
* The array should not contain any ancestors of this element, but could
* contain this element itself.
* @return {Array<!Element>}
* @return {!Array<!Element>}
* @public
*/
getDynamicElements() {
getDynamicElementContainers() {
return [];
}

Expand Down
4 changes: 2 additions & 2 deletions src/custom-element.js
Original file line number Diff line number Diff line change
Expand Up @@ -1360,8 +1360,8 @@ function createBaseCustomElementClass(win) {
* @return {Array<!Element>}
* @public
*/
getDynamicElements() {
return this.implementation_.getDynamicElements();
getDynamicElementContainers() {
return this.implementation_.getDynamicElementContainers();
}

/**
Expand Down
2 changes: 2 additions & 0 deletions src/form.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ const FORM_PROP_ = '__AMP_FORM';

/**
* @param {!Element} element
* @return {../extensions/amp-form/0.1/amp-form.AmpForm}
*/
export function formOrNullForElement(element) {
return element[FORM_PROP_] || null;
}

/**
* @param {!Element} element
* @param {!../extensions/amp-form/0.1/amp-form.AmpForm} form
*/
export function setFormForElement(element, form) {
element[FORM_PROP_] = form;
Expand Down
4 changes: 2 additions & 2 deletions src/service/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ export class Resource {

/**
* @param {!Element} element
* @return {?Resource}
* @return {Resource}
*/
static forElementOptional(element) {
return /** @type {?Resource} */ (element[RESOURCE_PROP_]);
return /** @type {Resource} */ (element[RESOURCE_PROP_]);
}

/**
Expand Down

0 comments on commit d3a0f42

Please sign in to comment.