Story education viewer messaging. #27194

gmajoulet · 2020-03-11T23:24:21Z

Story education viewer messaging:

Sends a message on build to display the tap + swipe navigation screen
Only sends messages when the doc gets visible, to avoid receiving messages from multiple prerendered docs
Stops propagation on touch events so they're not forwarded to the viewer, disabling the "swipe to next" feature (and the navigation hint overlay)
Adds an EDUCATION_STATE in the store service and expose it to viewers, so they can block swiping interactions while it's on if they're using native gestures

danielrozenberg

Approval for build-system/tasks/presubmit-checks.js

Enriqe · 2020-03-12T20:51:55Z

extensions/amp-story-education/0.1/test/test-amp-story-education.js

+        .resolves();
+
+      storyEducation.buildCallback();
+      await Promise.resolve(); // whenFirstVisible icrotask tick.


nit: s/icrotask/microtask

Trying to fix this with the "suggestion" feature 👀

Enriqe · 2020-03-12T20:56:34Z

extensions/amp-story-education/0.1/amp-story-education.js

@@ -104,6 +118,23 @@ export class AmpStoryEducation extends AMP.BaseElement {
      true /** useCapture */
    );

+    // Prevent touchevents from being forwarded through viewer messaging.


Should we remove these at some point?

It's actually what has been discussed, the issue being that the second screen says "swipe" but if we don't prevent the touchevents from being forwarded, users would be sent to the next story. :(

gmajoulet

PTAL

gmajoulet · 2020-03-13T12:09:27Z

extensions/amp-story-education/0.1/amp-story-education.js

@@ -104,6 +118,23 @@ export class AmpStoryEducation extends AMP.BaseElement {
      true /** useCapture */
    );

+    // Prevent touchevents from being forwarded through viewer messaging.


It's actually what has been discussed, the issue being that the second screen says "swipe" but if we don't prevent the touchevents from being forwarded, users would be sent to the next story. :(

extensions/amp-story-education/0.1/test/test-amp-story-education.js

gmajoulet · 2020-03-13T12:10:49Z

extensions/amp-story-education/0.1/test/test-amp-story-education.js

+        .resolves();
+
+      storyEducation.buildCallback();
+      await Promise.resolve(); // whenFirstVisible icrotask tick.


Trying to fix this with the "suggestion" feature 👀

newmuis · 2020-03-13T15:39:59Z

extensions/amp-story-education/0.1/amp-story-education.js

@@ -51,6 +52,11 @@ const buildNavigationEl = element => {
  `;
 };

+/** @enum {string} */
+const Screen = {
+  ONBOARDING_NAVIGATION: 'on',


nit: maybe be more descriptive about what this screen is? These names (the minified on version) are permanent since they'll be embedded in other viewers that we can't change, but I wonder if e.g. we experimented with multiple versions of onboarding, how we would tell them apart

Do you mean descriptive as in adding a comment in our codebase, or changing the on word to something more verbose?

If the latter, wdyt of something like: ONBOARDING_NAVIGATION_TAP_AND_SWIPE: 'ontas?

I meant the latter, and yep, your suggestion LGTM!

newmuis · 2020-03-13T15:41:36Z

extensions/amp-story-education/0.1/amp-story-education.js

+          )
+          .then(response => {
+            const shouldShow = !!(
+              response &&


Man, optional chaining would be great.....

newmuis · 2020-03-13T15:43:21Z

extensions/amp-story-education/0.1/amp-story-education.js

+        // screens, if/when needed.
+        this.viewer_
+          .sendMessageAwaitResponse(
+            'canShowScreens',


nit: I'm not entirely sure about making this plural; this seems like it could lead us to want to request multiple and cache the result, whereas I think the idea is to lookup can show at the instant the screen is to be displayed, in case things have changed

The inner object notation LGTM, but we should document

The motivation behind making it plural is that we want to know how many total screens we are going to display at a given time to:

Have accurate "Tip 1/n" labels

Not animate out / animate in again between different screens

But, this is one "screen" right? So (1) can't the tip labels be hardcoded? (2) would it do that?

Maybe I'm over engineering this: if tomorrow we add more screens, say audio controls, we might ask players if they want to show the navigation screens, and the audio controls screen on load.
If we don't ask that in the same message, there might be a delay in between both screens causing (2), and inaccurate "Tip 1/n" hardcoded labels

👍

I guess I'm also fine with an array and a comment on the format side that says we shouldn't cache these values

Sounds good.
The format side should always display all the screens right away (one by one), but not store the response to be used later.

extensions/amp-story-education/0.1/amp-story-education.js

gmajoulet · 2020-03-17T16:01:35Z

I'll merge this before the canary cut today cc @newmuis

* Story education viewer messaging. * s/icrotask/microtask * s/on/ontas * Add comment to not cache viewer responses. * Update tests.

gmajoulet requested review from newmuis and Enriqe March 11, 2020 23:24

amp-owners-bot bot requested a review from danielrozenberg March 11, 2020 23:24

googlebot added the cla: yes label Mar 11, 2020

danielrozenberg approved these changes Mar 12, 2020

View reviewed changes

Enriqe reviewed Mar 12, 2020

View reviewed changes

gmajoulet commented Mar 13, 2020

View reviewed changes

Enriqe approved these changes Mar 13, 2020

View reviewed changes

newmuis reviewed Mar 13, 2020

View reviewed changes

gmajoulet commented Mar 13, 2020

View reviewed changes

extensions/amp-story-education/0.1/amp-story-education.js Outdated Show resolved Hide resolved

gmajoulet force-pushed the education_navigation_messaging branch from 4b7bed4 to 43da571 Compare March 13, 2020 21:18

gmajoulet commented Mar 16, 2020

View reviewed changes

extensions/amp-story-education/0.1/amp-story-education.js Show resolved Hide resolved

gmajoulet added 4 commits March 16, 2020 15:07

Story education viewer messaging.

8e11bfc

s/icrotask/microtask

dca6efc

s/on/ontas

ae30a0f

Add comment to not cache viewer responses.

db760c4

gmajoulet force-pushed the education_navigation_messaging branch from d8a206b to db760c4 Compare March 16, 2020 19:07

Update tests.

07f0b98

gmajoulet merged commit 2ff07eb into ampproject:master Mar 17, 2020

ampprojectbot added the PR use: In Beta / Experimental label Mar 17, 2020

twintwox pushed a commit to twintwox/amphtml that referenced this pull request Mar 24, 2020

Story education viewer messaging. (ampproject#27194)

9cc8155

* Story education viewer messaging. * s/icrotask/microtask * s/on/ontas * Add comment to not cache viewer responses. * Update tests.

ampprojectbot added PR use: In Stable and removed PR use: In Stable labels Mar 25, 2020

ampprojectbot added the PR use: In Stable label Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Story education viewer messaging. #27194

Story education viewer messaging. #27194

gmajoulet commented Mar 11, 2020

danielrozenberg left a comment

Enriqe Mar 12, 2020

gmajoulet Mar 13, 2020

Enriqe Mar 12, 2020

gmajoulet Mar 13, 2020

gmajoulet left a comment

gmajoulet Mar 13, 2020

gmajoulet Mar 13, 2020

newmuis Mar 13, 2020

gmajoulet Mar 13, 2020

newmuis Mar 13, 2020

newmuis Mar 13, 2020

newmuis Mar 13, 2020

gmajoulet Mar 13, 2020

newmuis Mar 13, 2020

gmajoulet Mar 13, 2020

newmuis Mar 13, 2020 •

edited

gmajoulet Mar 16, 2020

gmajoulet commented Mar 17, 2020

Story education viewer messaging. #27194

Story education viewer messaging. #27194

Conversation

gmajoulet commented Mar 11, 2020

danielrozenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmajoulet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newmuis Mar 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmajoulet commented Mar 17, 2020

newmuis Mar 13, 2020 •

edited