✨ Add <amp-story-embed> AMP extension #25006
Conversation
| constructor(element) { | ||
| super(element); | ||
|
|
||
| this.embedManager_ = new AmpStoryEmbedManager( |
There was a problem hiding this comment.
I think it is safer to have components do 0 work in the constructor. Is there any downside to moving this to buildCallback?
If for some reason we need to instantiate the AmpStoryEmbedManager before the buildCallback, maybe we could use firstAttachedCallback
There was a problem hiding this comment.
Sounds good, I will change that. Just wondering: why do you think this is safer?
There was a problem hiding this comment.
Doing some work in the constructor of an element will run it as soon as the element is attached to the DOM, very likely during rendering.
Because the work is "hidden" in the other constructor of the EmbedManager, it would be easy to introduce latency or privacy issues by mistake, just because it is hard to realize when the code actually runs if you don't have context on the whole thing.
| @@ -683,6 +683,26 @@ | |||
| ".i-amphtml-story-loaded", | |||
| ], | |||
| }, | |||
| { | |||
| "flaky": true, // Skip this until shadow DOM the embeds support shadow DOM | |||
There was a problem hiding this comment.
Not sure I understand why the visual diff tests aren't working since we already support components in the shadow DOM like the bookend?
There was a problem hiding this comment.
For the core stories framework, we're actually not testing the shadow DOM code. When we create new shadow roots, we use a utility; that utility simply does not create a shadow root in test code.
amphtml/extensions/amp-story/1.0/utils.js
Lines 102 to 112 in f4fe32a
Since this code doesn't live in the amp-story extension, we don't use that util. We can use the same high-level approach, but to do so, we have to rewrite a bunch of things (all the styles break), so I figured it would be better to do as a separate PR.
|
Ping @jridgewell for OWNERS approval |
| /** @private {!Array<!HTMLAnchorElement>} */ | ||
| this.stories_ = Array.prototype.slice.call(element.querySelectorAll('a')); | ||
| /** @private {?Array<!HTMLAnchorElement>} */ | ||
| this.stories_; |
There was a problem hiding this comment.
You need to set these to null. As is, these don't actually create the field on the object, which causes polymorphism.
| return; | ||
| } | ||
|
|
||
| this.stories_ = Array.prototype.slice.call( |
There was a problem hiding this comment.
We have a helper for this in src/array.js, I think.
| const embedImpl = new AmpStoryEmbed(doc, embed); | ||
| embedImpl.build(); | ||
| if (!isAMP) { | ||
| const doc = self.document; |
There was a problem hiding this comment.
This gets run very early in the page lifecycle, there's no guarantee that you'll find the amp-story-embed element.
This adds an AMP component that has the same API/behavior as the non-AMP component, for embedding AMP stories.
As a side note, visual diff tests don't work for anything in shadow DOM, so they're disabled for now.
This also puts the AMP and non-AMP variants behind an experiment.