Skip to content
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

🐛 [amp-story] Build pagination buttons for pre-rendered stories #30503

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Oct 2, 2020

While working on #30031 I noticed pagination buttons weren't being built for documents that were initially prerendered. This is because the layoutCallback() was being short circuited here for prerendered documents:

// Story is being prerendered: resolve the layoutCallback when the first
// page is built. Other pages will only build if the document becomes
// visible.
if (!this.getAmpDoc().hasBeenVisible()) {
return whenUpgradedToCustomElement(firstPageEl).then(() => {
return firstPageEl.whenBuilt();
});
}

Which is right before the pagination buttons are built.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Oct 2, 2020

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

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/pagination-buttons.css

@processprocess
Copy link
Contributor

Thanks for catching this Enrique! I see how this ordering is important for pre-rendering now.

@gmajoulet
Copy link
Contributor

Yep this was done on purpose as the pagination buttons are desktop only, and there's no platform prerendering documents with a desktop view.
Is this something the open source Web Story Player needs?

@gmajoulet
Copy link
Contributor

Oh ok got it, they'd actually never build. Do you need to prerender them or do you need them to build once out of prerender? Let's think of when we need them, since we want to do as little as possible during prerender. We have to be super cautious about CPU cycles at this point.

@Enriqe
Copy link
Contributor Author

Enriqe commented Oct 6, 2020

Yeah, this is no big deal because I don't think we have a platform where we pre-render a story and then make it visible in desktop view. But it is needed for the open source web player since this is a very present use case.

Do you need to prerender them or do you need them to build once out of prerender?

We could wait building them until after ampDoc.whenFirstVisible() finishes, wdyt?

@Enriqe
Copy link
Contributor Author

Enriqe commented Oct 6, 2020

I moved the building of the buttons to inside storyLayoutPromise, which waits for the document to be visible before running the code inside of it:

const storyLayoutPromise = Promise.all([
this.getAmpDoc().whenFirstVisible(), // Pauses execution during prerender.

PTAL @gmajoulet

@gmajoulet
Copy link
Contributor

Yep that looks great, and that will actually make the accessibility features @processprocess worked on reachable for players using prerendering, like the Discover integration.

@Enriqe Enriqe merged commit 386f6d2 into ampproject:master Oct 6, 2020
@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation Oct 6, 2020
@gmajoulet gmajoulet moved this from In progress to Done in wg-stories Sprint Oct 6, 2020
@gmajoulet gmajoulet removed this from Done in wg-stories Sprint Oct 9, 2020
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
…roject#30503)

* build pagination buttons for prerendered stories

* prettier

* build pagination buttons after doc is visible
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants