♻️ Story: refactor to treat story ad pages as story pages by powerivq · Pull Request #37940 · ampproject/amphtml

powerivq · 2022-03-23T06:13:32Z

This is so that we can put ads into the progress bar more eloquently.

amp-owners-bot · 2022-03-23T06:13:43Z

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/progress-bar.js
extensions/amp-story/1.0/test/test-amp-story-viewer-messaging-handler.js
extensions/amp-story/1.0/test/test-amp-story.js
extensions/amp-story/1.0/test/test-variable-service.js
extensions/amp-story/1.0/variable-service.js

mszylkowski

Would be great to have a list of things that you want to update, I've caught a few that you need to include, or keep track of them somewhere for future implementation

extensions/amp-story/1.0/test/test-amp-story.js

extensions/amp-story/1.0/variable-service.js

mszylkowski · 2022-03-29T14:55:31Z

extensions/amp-story/1.0/variable-service.js

+    this.variables_[AnalyticsVariable.STORY_PAGE_INDEX] =
+      pageIndex - adsBeforePage;
+
+    const numberOfPages = pageIds.length;


Shouldn't the number of pages here not contain the ad pages? So that the variables don't care about the ad pages.

My suggestion is at the beginning do pageIds = storeService.get().filter((id) => id.startsWith...) and then you just work with the pageIds without the ads.

You can also do if pageId.startsWith('i-amphtml-ad') {return} so that you don't run this variable update if the current page is an ad

The problem is that we have this CURRENT_PAGE_INDEX, which is the index in pageIds (incl. ads). If we filter out non-ad pages beforehand, we won't be able to figure out how many ad pages we should exclude from CURRENT_PAGE_INDEX to ensure that analytics will get the same number (page index without regards to ads) as before.

mszylkowski · 2022-03-29T14:58:03Z

extensions/amp-story/1.0/progress-bar.js

-          });
+          /** @type {!Array} */ (pageIds)
+            // Do not show progress bar for the ad page.
+            .filter((id) => !id.startsWith('i-amphtml-ad-'))


Would this not add into the progress bar the section for the ad pages? That's exactly what we want to show. You'd also need to update buildSegmentEl_

as a first step we don't show any UI change:

Refactoring

UI change

Yes, we want to do a pure refactoring before having another PR to show ad segments in the progress bar.

gmajoulet

(Did not review the variable service yet)

extensions/amp-story/1.0/amp-story.js

extensions/amp-story/1.0/amp-story-store-service.js

gmajoulet · 2022-03-29T20:08:20Z

extensions/amp-story/1.0/amp-story.js

+    this.storeService_.dispatch(Action.INSERT_AD_PAGE, {
+      pageBeforeId,
+      pageToBeInsertedId,
+    });


All the logic inside of the INSERT_AD_PAGE is not needed, you already have the pages array with all the IDs.
Plz delete the action, and all you need is:

this.storeService_.dispatch(Action.SET_PAGE_IDS, this.pages_.map((el) => el.id));

This won't work as ad page elements are not ordered.

gmajoulet · 2022-03-29T20:09:56Z

extensions/amp-story/1.0/progress-bar.js

-          });
+          /** @type {!Array} */ (pageIds)
+            // Do not show progress bar for the ad page.
+            .filter((id) => !id.startsWith('i-amphtml-ad-'))


as a first step we don't show any UI change:

Refactoring

UI change

extensions/amp-story/1.0/progress-bar.js

extensions/amp-story/1.0/test/test-amp-story.js

powerivq · 2022-03-30T20:25:51Z

@gmajoulet @mszylkowski Updated for another look.

extensions/amp-story/1.0/amp-story.js

mszylkowski

In order to ensure the behavior stays consistent can you add some more tests? Eg: check variable service contains the correct index of a page that comes after an ad; check the variable service contains the correct page length, etc

mszylkowski · 2022-04-01T14:23:00Z

extensions/amp-story/1.0/amp-story.js

    }

+    // Adjust the ad page's position in this.adPages_ array to reflect the actual.
+    const adPageIndex = this.getPageIndexById(pageToBeInsertedId);


This is already calculated above: pageToBeInserted

This line gets the index of the page, so it is different from pageToBeInserted.

mszylkowski · 2022-04-01T14:23:43Z

extensions/amp-story/1.0/amp-story.js

+      pageToBeInserted
+    );
+
+    this.storeService_.dispatch(


From this reordering, why not insert the ad in the right place from the start, if possible?

We query and insert the ad page early, way before we know where to insert it. We only determine to inject it one page before the ad page is scheduled to show. At that point, we would need to move the DOM to have it in the right place.

I suspect there could be a performance hit, but not sure if it is the reason why we don't do it. I am for it but let's leave this out of this PR.

Right this is so that we can create/preload the ad element before we know if it will actually load, or even if the server will respond with an ad.

mszylkowski · 2022-04-01T14:26:41Z

extensions/amp-story/1.0/test/test-amp-story.js

+          ]);
+          await story.layoutCallback();
+          // Getting all the AmpStoryPage objets.
+          const pageElements =


pageElements is the return of createStoryWithPages, you can get it from there

mszylkowski · 2022-04-01T14:45:39Z

extensions/amp-story/1.0/variable-service.js

+
+    const numberOfPages = pageIds.length;
+    if (numberOfPages > 0) {
+      if (numberOfPages === 1) {


This if statement can be cleaner: if (numberOfPages === 1) {} else if (numberOfPages > 1) {}

calebcordry · 2022-04-04T19:18:50Z

extensions/amp-story/1.0/amp-story.js

      nextPageEl.setAttribute(Attributes.RETURN_TO, pageToBeInsertedId);
    }

+    // Adjust the ad page's position in this.adPages_ array to reflect the actual.


nit: this.pages_

calebcordry · 2022-04-04T19:33:51Z

extensions/amp-story/1.0/amp-story.js

+      pageToBeInserted
+    );
+
+    this.storeService_.dispatch(


Right this is so that we can create/preload the ad element before we know if it will actually load, or even if the server will respond with an ad.

ampprojectbot · 2022-04-04T20:30:36Z

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
If unsure, @-mention ampproject/wg-approvers

amp-owners-bot bot requested a review from gmajoulet March 23, 2022 06:13

powerivq removed the request for review from gmajoulet March 23, 2022 06:14

powerivq changed the title ~~Story: refactor to treat story ad pages as story pages~~ ♻️ Story: refactor to treat story ad pages as story pages Mar 23, 2022

powerivq requested a review from mszylkowski March 28, 2022 20:03

powerivq force-pushed the story-list-refactor branch from 431f02c to 6e3078b Compare March 28, 2022 20:43

powerivq requested a review from calebcordry March 28, 2022 20:43

powerivq force-pushed the story-list-refactor branch from 6e3078b to 948999e Compare March 29, 2022 06:52

Story: refactor to treat story ad pages as story pages

3adbe51

powerivq force-pushed the story-list-refactor branch from 948999e to 3adbe51 Compare March 29, 2022 09:41

mszylkowski reviewed Mar 29, 2022

View reviewed changes

gmajoulet reviewed Mar 29, 2022

View reviewed changes

powerivq requested review from gmajoulet and mszylkowski March 30, 2022 00:20

gmajoulet reviewed Mar 31, 2022

View reviewed changes

extensions/amp-story/1.0/amp-story.js Outdated Show resolved Hide resolved

Review comments

664503e

powerivq force-pushed the story-list-refactor branch from f490e46 to 664503e Compare March 31, 2022 22:07

powerivq requested a review from gmajoulet March 31, 2022 23:28

mszylkowski reviewed Apr 1, 2022

View reviewed changes

Add tests for variable service

889cdf4

powerivq force-pushed the story-list-refactor branch from 778859d to 889cdf4 Compare April 1, 2022 20:41

mszylkowski approved these changes Apr 4, 2022

View reviewed changes

calebcordry approved these changes Apr 4, 2022

View reviewed changes

powerivq merged commit e75f657 into ampproject:main Apr 4, 2022

powerivq deleted the story-list-refactor branch April 4, 2022 20:17

jshamble mentioned this pull request Apr 8, 2022

wg-stories Status Update 2022-04-08 ampproject/wg-stories#101

Open

powerivq mentioned this pull request Apr 8, 2022

🔥 Fix progress bar overflow mode bug #38053

Merged

ampprojectbot added the PR use: In Beta / Experimental label Apr 21, 2022

ampprojectbot added the PR use: In Stable label Apr 26, 2022

ampprojectbot added the PR use: In LTS label May 12, 2022

Conversation

powerivq commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mszylkowski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

gmajoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

powerivq commented Mar 30, 2022

Uh oh!

Uh oh!

mszylkowski 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ampprojectbot commented Apr 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

powerivq commented Mar 23, 2022 •

edited

Loading

amp-owners-bot bot commented Mar 23, 2022 •

edited

Loading