-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
♻️ Story: refactor to treat story ad pages as story pages #37940
Conversation
Hey @gmajoulet, @newmuis! These files were changed:
|
431f02c
to
6e3078b
Compare
6e3078b
to
948999e
Compare
948999e
to
3adbe51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
this.variables_[AnalyticsVariable.STORY_PAGE_INDEX] = | ||
pageIndex - adsBeforePage; | ||
|
||
const numberOfPages = pageIds.length; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}); | ||
/** @type {!Array} */ (pageIds) | ||
// Do not show progress bar for the ad page. | ||
.filter((id) => !id.startsWith('i-amphtml-ad-')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a first step we don't show any UI change:
- Refactoring
- UI change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we want to do a pure refactoring before having another PR to show ad segments in the progress bar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Did not review the variable service yet)
this.storeService_.dispatch(Action.INSERT_AD_PAGE, { | ||
pageBeforeId, | ||
pageToBeInsertedId, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work as ad page elements are not ordered.
}); | ||
/** @type {!Array} */ (pageIds) | ||
// Do not show progress bar for the ad page. | ||
.filter((id) => !id.startsWith('i-amphtml-ad-')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a first step we don't show any UI change:
- Refactoring
- UI change
@gmajoulet @mszylkowski Updated for another look. |
f490e46
to
664503e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -2375,6 +2360,21 @@ export class AmpStory extends AMP.BaseElement { | |||
nextPageEl.setAttribute(Attributes.RETURN_TO, pageToBeInsertedId); | |||
} | |||
|
|||
// Adjust the ad page's position in this.adPages_ array to reflect the actual. | |||
const adPageIndex = this.getPageIndexById(pageToBeInsertedId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already calculated above: pageToBeInserted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line gets the index of the page, so it is different from pageToBeInserted
.
pageToBeInserted | ||
); | ||
|
||
this.storeService_.dispatch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this reordering, why not insert the ad in the right place from the start, if possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
]); | ||
await story.layoutCallback(); | ||
// Getting all the AmpStoryPage objets. | ||
const pageElements = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pageElements
is the return of createStoryWithPages
, you can get it from there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
const numberOfPages = pageIds.length; | ||
if (numberOfPages > 0) { | ||
if (numberOfPages === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement can be cleaner: if (numberOfPages === 1) {} else if (numberOfPages > 1) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
778859d
to
889cdf4
Compare
@@ -2375,6 +2360,21 @@ export class AmpStory extends AMP.BaseElement { | |||
nextPageEl.setAttribute(Attributes.RETURN_TO, pageToBeInsertedId); | |||
} | |||
|
|||
// Adjust the ad page's position in this.adPages_ array to reflect the actual. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this.pages_
pageToBeInserted | ||
); | ||
|
||
this.storeService_.dispatch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Warning: disparity between this PR Percy build and its 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 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 /
|
This is so that we can put ads into the progress bar more eloquently.