-
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
✨Update storyNextUp to auto-advance on all pages. #33600
Conversation
Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:
|
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.
Thanks for the PR @zaparent :) Do you mind adding a couple of unit tests please?
You can grab inspiration from these test files:
Advancement related:
https://github.com/ampproject/amphtml/blob/main/extensions/amp-story/1.0/test/test-page-advancement.js#L36
story-page and advancement:
https://github.com/ampproject/amphtml/blob/main/extensions/amp-story/1.0/test/test-amp-story-page.js#L137
Checking for a viewer param:
amphtml/extensions/amp-story/1.0/test/test-amp-story.js
Lines 116 to 120 in 6f57dc0
const viewer = Services.viewerForDoc(env.ampdoc); | |
env.sandbox | |
.stub(viewer, 'hasCapability') | |
.withArgs('swipe') | |
.returns(hasSwipeCapability); |
@Enriqe thank you for pushing for tests! We have two followup PRs to make this one work:
The second one will be rather complex and will change most of this implementation. Let's require unit tests for this PR, and unblock this one as the implementation (and the tests) would change. |
isLastPage_()
andisLastStoryPage_
getFirstVideo_()
maybeSetStoryNextUp_()
to apply to all pagesmaybeSetStoryNextUp_()
to prefer using the first video when possible