add mutateElement to amp-geo for layers support. #15226
Conversation
extensions/amp-geo/0.1/amp-geo.js
Outdated
| // Let the runtime know we just if we added/changed classes that | ||
| // may case a re-layout. | ||
| if (this.mutated_) { | ||
| return this.mutateElement(() => { |
There was a problem hiding this comment.
This needs to be down where the body mutations happen.
There was a problem hiding this comment.
The issue I had is if I do it in the buildCallback then it fails because the element isn't attached ... layoutCallback was the earliest that it would work. This may be me not quite understanding how it all hangs together.
There was a problem hiding this comment.
then it fails because the element isn't attached
How's it not attached?
There was a problem hiding this comment.
the getResources() in baseElement.measureMutateElement() fails with "'no resources yet, since element is not attached"
There was a problem hiding this comment.
Stack please. It should definitely have connected by the time #buildCallback runs.
There was a problem hiding this comment.
Ah, found it, I wasn't attaching the element in my unit tests :-( Amended code shortly.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| @@ -204,6 +205,8 @@ export class AmpGeo extends AMP.BaseElement { | |||
| return waitForBodyPromise(doc).then(() => { | |||
| self.findCountry_(doc); | |||
There was a problem hiding this comment.
Nit: self should just be this.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| @@ -214,10 +217,17 @@ export class AmpGeo extends AMP.BaseElement { | |||
| states.ISOCountry = self.country_; | |||
|
|
|||
| for (let group = 0; group < self.matchedGroups_.length; group++) { | |||
extensions/amp-geo/0.1/amp-geo.js
Outdated
| self.mutateElement(() => { | ||
| // add classes to <body> | ||
| doc.body.classList.add.apply(doc.body.classList, classesToAdd); | ||
| }, doc.body); |
There was a problem hiding this comment.
The config.AmpBind stuff should be in this block, too.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| } | ||
| doc.body.classList.add(COUNTRY_PREFIX + this.country_); | ||
| doc.body.classList.remove('amp-geo-pending'); | ||
| /* @type {Array<string>} */ |
There was a problem hiding this comment.
Nit: Variables don't need type annotations, only properties.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| self.mutateElement(() => { | ||
| // Always remove the pending class | ||
| classesToRemove.push('amp-geo-pending'); | ||
| doc.body.classList.remove.apply( |
There was a problem hiding this comment.
Nit: Save doc.body.classList to a variable.
extensions/amp-geo/0.1/amp-geo.js
Outdated
| const classList = doc.body.classList; | ||
| // Always remove the pending class | ||
| classesToRemove.push('amp-geo-pending'); | ||
| classList.remove.apply(classList, classesToRemove); |
There was a problem hiding this comment.
Eek, just remembered that IE doesn't support multi-parameter add and remove: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Browser_compatibility
Adds
mutateElementinlayoutCallbackto let layers know when change the body classes. Per #14846 and #15158