amp-bind: Don't depend on template location to locate dynamic elements #8367
Conversation
extensions/amp-bind/0.1/bind-impl.js
Outdated
| if (typeof element.getDynamicElements === 'function') { | ||
| dynamicElements = element.getDynamicElements(); | ||
| } else if (element.tagName === 'FORM') { | ||
| // FORM is not an amp element, so it doesn't have the getter. |
There was a problem hiding this comment.
We could add a static method to AmpForm that returns the related instance for a given form element. Similar to what we do with Resource#forElement
There was a problem hiding this comment.
Done.
When I tried to add the type annotation
@return {?../extensions/amp-form/0.1/amp-form.AmpForm}
on the functions in src/form.js I got the following error:
WARNING - Failed to load module "../extensions/amp-form/0.1/amp-form.js"
Any idea what's happening here?
There was a problem hiding this comment.
Probably need to add it to the sources list for Closure type checker in build-system/tasks/compile.js.
|
/to @choumx @jridgewell |
src/base-element.js
Outdated
| * 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>} |
There was a problem hiding this comment.
!Array<!Element>. Classes are nullable by default in Closure typing.
src/base-element.js
Outdated
| * @return {Array<!Element>} | ||
| * @public | ||
| */ | ||
| getDynamicElements() { |
There was a problem hiding this comment.
Maybe getDynamicElementContainers is a bit more precise? We don't actually return every dynamic element.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| if (typeof element.getDynamicElements === 'function') { | ||
| dynamicElements = element.getDynamicElements(); | ||
| } else if (element.tagName === 'FORM') { | ||
| // FORM is not an amp element, so it doesn't have the getter. |
There was a problem hiding this comment.
Probably need to add it to the sources list for Closure type checker in build-system/tasks/compile.js.
extensions/amp-form/0.1/amp-form.js
Outdated
| const errorDiv = | ||
| this.form_./*OK*/querySelector(`[${FormState_.SUBMIT_ERROR}]`); | ||
| successDiv && dynamicElements.push(successDiv); | ||
| errorDiv && dynamicElements.push(errorDiv); |
There was a problem hiding this comment.
IMO it's more readable to use conditionals when your statements have side effects. || or the ternary operator are typically used for returning values without side effects.
src/service/resource.js
Outdated
| @@ -89,7 +89,7 @@ export class Resource { | |||
| * @return {?Resource} | |||
| */ | |||
| static forElementOptional(element) { | |||
| return /** @type {!Resource} */ (element[RESOURCE_PROP_]); | |||
| return /** @type {?Resource} */ (element[RESOURCE_PROP_]); | |||
There was a problem hiding this comment.
Nit: ? unnecessary. Objects are nullable by default.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| } 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) { |
There was a problem hiding this comment.
Not sure we'd ever expect this to be null. May want to assert here.
kmh287
force-pushed
the
bind_dynamic_elements
branch
from
acdc68d to
d3a0f42
Compare
kmh287
force-pushed
the
bind_dynamic_elements
branch
from
d3a0f42 to
f415c30
Compare
| */ | ||
|
|
||
| /** @const {string} */ | ||
| const FORM_PROP_ = '__AMP_FORM'; |
There was a problem hiding this comment.
I wonder if it's beneficial to extend this beyond just Forms. As in, do we expect there'll be another element that has an AMP class equivalent, but doesn't use it directly.
There was a problem hiding this comment.
It's definitely a useful pattern. My understanding is that bind doesn't support any other elements that behave this way but might in the future.
There was a problem hiding this comment.
Seems unlikely since IIRC Dima somewhat regrets not having forms follow the same custom element pattern.
|
@jridgewell good to go? |
Added a method to base-element for an element to report any elements it "owns" that are containers for dynamic contents. For instance,
<div items>on an amp-live-list can have elements added/removed from its child list and so amp-live-list should return that div. The returned elements will be observed for mutations by bind. I'm not thrilled with the method name,getDynamicElementsso I'm open to renaming.I've also changed
bind-implto depend on this new method instead of the locations of templates. One caveat is forms;amp-formisn't an element, and so I've implemented custom behavior for the success/failure divs.Fixes #8341
/to @choumx @jridgewell