✨ [amp-story-embed] Adds messaging, implements build/layoutcallbacks by Enriqe · Pull Request #26305 · ampproject/amphtml

Enriqe · 2020-01-10T20:36:31Z

Partial for #24539

This PR adds:

Messaging that is needed for communication between the viewer and the STAMPs.

Adds an intersection observer that calls layoutCallback() with a fallback that uses a scroll listener.

Things I'm not sure about:

Since I'm installing a new package (@ampproject/viewer-messaging), yarn.lock has been updated, but I don't know if that should be in my local gitignore or if it should actually be checked in.

Issue tracker #26308

src/amp-story-embed.js

gmajoulet

As commented below, I think some of this PR is taking us further from the end goal.
What we want is a piece of code that loads or prerenders a story, that we can call again and again as the user progresses through the feed of story.
This PR moves away from this and loads all the stories at the same time, which is a piece of code we'll never need in the final library.

examples/amp-story/embed.html

package.json

src/amp-story-embed.js

Enriqe · 2020-01-14T23:17:20Z

Decided to move the loading one iframe + prerendering the next ones logic to another PR, to keep this one small and focused.

PTAL.

gmajoulet

Can you add some basic unit tests? I know we want to ship this asap but I believe it'll save us time :)

src/amp-story-embed.js

Enriqe · 2020-01-16T20:17:50Z

PTAL @gmajoulet

also need owners approval from @jridgewell / @choumx for build-system/* and package.json

src/amp-story-embed.js

gmajoulet · 2020-01-16T22:32:35Z

src/amp-story-embed.js

+    });
+
+    let inputUrl = addParamsToUrl(href, params);
+    inputUrl = inputUrl.replace('amp_js_v=0.1', 'amp_js_v=0.1#');


Isn't this going to leave an extra & after the #?

Also, this won't work as addParamsToUrl seems to be adding the query parameter before the hash. You'd break the canonical URL by having the original hash at the very end of the URL.

Good catch! I am now extracting the original fragment param first before prepending it again after adding the new parameters.

test/unit/test-amp-story-embed.js

Enriqe · 2020-01-17T20:37:05Z

PTAL @gmajoulet

Enriqe · 2020-01-22T19:40:41Z

Friendly ping

gmajoulet

Looking good :))

gmajoulet · 2020-01-22T23:10:58Z

src/amp-story-embed-manager.js

+    this.win_ = win;
+
+    /** @private {?AmpStoryEmbed} */
+    this.currentEmbed_ = null;


Nit: we should not write code that's only used for unit testing purposes (this and the getEmbed() method). Do you have another way of writing your test that wouldn't need this piece of code?
getEmbed returns you a pretty arbitrary embed (the last one), and currentEmbed doesn't make sense if there are two embeds side to side in the page

gmajoulet · 2020-01-22T23:11:33Z

src/amp-story-embed-manager.js

+      return;
+    }
+
+    this.layoutFallback_(embedImpl);


Might be easier to read with an early return after this one line?

src/amp-story-embed-manager.js

gmajoulet · 2020-01-22T23:19:44Z

src/amp-story-embed.js

+    this.messagingFor_ = {};
+
+    /** @private {!Array<!Element>} */
+    this.iframes_ = [];


A few thoughts for another PR: I'm almost wondering if we should just store the story URLs instead of the iframes, so you can rotate through 3 or 5 iframe elements. Storing the actual iframes will make it harder/impossible to re-use the iframe elements.

Yep, this was one of the approaches I was thinking for my next PRs when enabling multiple stories and I agree we should rotate through a fixed amount of iframes.

Enriqe

PTAL

Enriqe · 2020-01-23T16:56:17Z

src/amp-story-embed-manager.js

+    this.win_ = win;
+
+    /** @private {?AmpStoryEmbed} */
+    this.currentEmbed_ = null;


Enriqe · 2020-01-23T16:56:27Z

src/amp-story-embed-manager.js

+      return;
+    }
+
+    this.layoutFallback_(embedImpl);


src/amp-story-embed-manager.js

gmajoulet

Great! 👍

…ayout logic.

Enriqe · 2020-01-23T18:32:37Z

/to @jridgewell @choumx for OWNERS approval

src/amp-story-embed-manager.js

Enriqe · 2020-01-24T15:38:38Z

PTAL @jridgewell @choumx

Enriqe · 2020-01-27T15:47:07Z

Ping @jridgewell

jridgewell

One last question, but LGTM otherwise.

src/amp-story-embed-manager.js

Enriqe · 2020-01-28T22:42:39Z

Thanks all, merging!

Enriqe requested review from gmajoulet and newmuis January 10, 2020 20:36

googlebot added the cla: yes label Jan 10, 2020

amp-owners-bot bot requested a review from dreamofabear January 10, 2020 20:36

Enriqe changed the title ~~✨ Adds messaging, implements build/layoutcallbacks~~ ✨ [amp-story-embed] Adds messaging, implements build/layoutcallbacks Jan 10, 2020

Enriqe mentioned this pull request Jan 10, 2020

[Issue tracker] 👓 amp-story-player #26308

Closed

newmuis reviewed Jan 10, 2020

View reviewed changes

gmajoulet reviewed Jan 10, 2020

View reviewed changes

Enriqe force-pushed the story-embed2messaging branch from ab0e69a to 28b71cc Compare January 14, 2020 23:24

gmajoulet reviewed Jan 15, 2020

View reviewed changes

Enriqe force-pushed the story-embed2messaging branch from b024467 to 578c398 Compare January 16, 2020 19:29

Enriqe force-pushed the story-embed2messaging branch from c4b1d85 to f6f7c6e Compare January 16, 2020 20:31

gmajoulet reviewed Jan 16, 2020

View reviewed changes

Enriqe force-pushed the story-embed2messaging branch from f6f7c6e to bb6eb39 Compare January 17, 2020 19:45

gmajoulet reviewed Jan 22, 2020

View reviewed changes

Enriqe commented Jan 23, 2020

View reviewed changes

gmajoulet approved these changes Jan 23, 2020

View reviewed changes

Enriqe added 9 commits January 23, 2020 13:10

wip

52f22c1

add messaging, implements build/layoutcallbacks

6994980

prerender 2n viewports away, add IO fallback, remove logging, defer l…

034e43a

…ayout logic.

pull logic to functions, make url logic more robust.

9abbbb3

adds basic unit tests

b70375e

add viewer messaging as a dependency

76aa895

fix test

0f128af

split into files, fix url fragment param, update test

8a6706d

clean up

a080226

fix rebase

b466435

Enriqe force-pushed the story-embed2messaging branch from cdd32c6 to b466435 Compare January 23, 2020 18:15

update owners for amp-story-embed-manager

8e4689c

jridgewell reviewed Jan 23, 2020

View reviewed changes

correctly check for IO

9bcb671

jridgewell approved these changes Jan 27, 2020

View reviewed changes

src/amp-story-embed-manager.js Outdated Show resolved Hide resolved

src/amp-story-embed-manager.js Outdated Show resolved Hide resolved

change for existing throttle

5dd71f7

Enriqe merged commit 0b9623c into ampproject:master Jan 28, 2020

Enriqe deleted the story-embed2messaging branch January 28, 2020 22:42

ampprojectbot added PR use: In Beta / Experimental and removed PR use: In Beta / Experimental labels Feb 5, 2020

ampprojectbot added the PR use: In Stable label Feb 25, 2020

Conversation

Enriqe commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enriqe commented Jan 14, 2020

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Enriqe commented Jan 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Enriqe commented Jan 17, 2020

Uh oh!

Enriqe commented Jan 22, 2020

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enriqe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Enriqe commented Jan 10, 2020 •

edited

Loading