Make advance-to public #20116
Make advance-to public #20116
Conversation
|
We likely also want this behind an experiment, so we can control when it goes live 😄 We can use one experiment to guard everything, if that's easier |
|
Adding @calebcordry since this affects ads code |
|
Do we want to: (1) merge this whenever we want to launch the feature? |
|
@bramanudom this should be fine, but would you mind testing manually to be sure? You can use the |
|
@gmajoulet I think adding these small changes to the same branching experiment would be good. Not sure if it would make a big difference, but it would be great to be able to test/ see how the changes work together. If that sounds good, I'll add in the experiment statements @calebcordry We talked online but I did a manual test:
and it all seems to be working as expected. I will be adding a unit test for amp-story tests in this PR as well as validation that this works for branching purposes. |
|
|
||
| if (isExperimentOn(this.win, 'amp-story-branching')) { | ||
| Attributes[ADVANCE_TO] = 'advance-to'; | ||
| } |
There was a problem hiding this comment.
Can we define another attribute (ie: Attributes.BRANCHING_ADVANCE_TO, or PUBLIC_ADVANCE_TO, ...) instead, that we'd use here and for the other occurrences? It's pretty risky to change the values of an enum :(
| .element.setAttribute('advance-to', 'page-3'); | ||
|
|
||
| story.activePage_.element.dispatchEvent( | ||
| new MouseEvent('click', {clientX: 200})); |
|
|
||
| /** @enum {string} */ | ||
| const Attributes = { | ||
| let Attributes = { |
There was a problem hiding this comment.
nit: Can't we leave this as const since it is an obj?
| const nextPageEl = nextPage.element; | ||
| const nextPageId = nextPageEl.id; | ||
| pageToBeInsertedEl.setAttribute(Attributes.ADVANCE_TO, nextPageId); | ||
| pageToBeInsertedEl.setAttribute(Attributes.advanceAttr, nextPageId); |
There was a problem hiding this comment.
Should we also change the one in the next_ method for this PR to work?
There was a problem hiding this comment.
Thank you for catching this! I never caught it in testing because this check addresses the case where the last page has the private attribute.
I'm assuming that the check for the private attribute was originally added in the case where an ad should follow the last page. I made the change/added the check, but I'm also not sure what the use case for allowing the last page of a story to have a public advance-to attribute would be. It seems like it would more likely than not create an infinite cycle since a story wouldn't be able to 'end'.
There was a problem hiding this comment.
I am a bit confused here. This is the method that amp-story-auto-ads calls when it wants to add a new page to the story. So if we have a story like page1 -> page2 we can change it to page1 -> ad -> page2. All this logic is just trying to copy any pointers from page1 to the ad so that the flow is the same.
There was a problem hiding this comment.
Sorry for the confusion, I definitely could've been more clear. Gabriel brought up changing the code in next_ that uses ADVANCE_TO as well, and my response was for the logic in the that method and not in insertPage.
There was a problem hiding this comment.
I see, I think that the the idea there was just future proofing knowing that this eventually may be used for non linear stories, but @gmajoulet is right, as it is written now it won't respect the experiment.
There was a problem hiding this comment.
(the ad can't be the last page of a story anyway)
amphtml/extensions/amp-story-auto-ads/0.1/amp-story-auto-ads.js
Lines 682 to 684 in 5ce6f62
|
I think this is ready for merging! @gmajoulet |
* make advance-to a public attribute * added a tests, and put the changes behind an experiment check * addressed code review changes * added public advance-to check in next * add TODOs for cleaning up the experiment * Revert "add TODOs for cleaning up the experiment" This reverts commit 088a0c5. * fixed merge conflicts * fixed lint issues * fixed typo
#20083
Publishers should be able to specify what page should follow a given page element. This has already existed/ been correctly implemented in the form of
i-amphtml-advance-to. Removing just thei-amphtmlshould be sufficient for making this usable by publishers.