Skip to content
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

Mutate amp-ad inside mutation phase #13250

Merged
merged 2 commits into from Mar 7, 2018

Conversation

jridgewell
Copy link
Contributor

Detected with black magic.

@jridgewell jridgewell merged commit d3dd17f into ampproject:master Mar 7, 2018
@jridgewell jridgewell deleted the amp-ad-mutate branch March 7, 2018 02:15
@@ -32,7 +32,8 @@ export class AmpAdUIHandler {
this.doc_ = baseInstance.win.document;

if (!baseInstance.getFallback()) {
this.addDefaultUiComponent_('fallback');
const fallback = this.addDefaultUiComponent_('fallback');
this.baseInstance_.element.appendChild(fallback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got an regression here:

[Resource] failed to build: amp-embed#1 TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
    at new AmpAdUIHandler (amp-ad-ui.js:36)
    at AmpAd3PImpl.buildCallback (amp-ad-3p-impl.js:169)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh.

RanAbram pushed a commit to RanAbram/amphtml that referenced this pull request Mar 12, 2018
* Mutate amp-ad inside mutation phase

Detected with [black magic](ampproject#13223).

* Test updates
@jridgewell jridgewell added this to TODO in Layers via automation Apr 9, 2018
@jridgewell jridgewell moved this from TODO to Done (Sub tasks) in Layers Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Layers
  
Done (Mutation Subtasks)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants