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

💅 Live story polishes #23074

Merged
merged 6 commits into from Jun 28, 2019
Merged

💅 Live story polishes #23074

merged 6 commits into from Jun 28, 2019

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Jun 27, 2019

Part of #21714

  • Installs amp-live-list extension automatically.
  • Updates pagination buttons in 3-panels view. Closes Pagination buttons aren't updated when a live update comes in #22770.
  • Updates distance attributes on new pages.
  • Supports adding a new page in the middle of a story & updates progress bar accordingly.
  • Stops trying to append the new pages on the LiveStoryManger and leaves that to the amp-live-list.
  • Adds an example live story.

extensions/amp-story/1.0/amp-story-store-service.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/amp-story.js Show resolved Hide resolved
extensions/amp-story/1.0/live-story-manager.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/live-story-manager.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/progress-bar.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/progress-bar.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/progress-bar.js Outdated Show resolved Hide resolved
Services.extensionsFor(this.ampdoc_.win).installExtensionForDoc(
this.ampdoc_,
'amp-live-list'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should one of two things here:

  1. Enforce that the extension is present at validation level
  2. If we decide to lazy load it, we should do so at a better time, when all the first pages assets are loaded. There's a custom even that's dispatched for that

My vote goes to (1), since we really want publishers to understand that using a live-story is expensive: it loads and parses (!!!) the full HTML document every 15s for every single user viewing it, and loads more code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @newmuis prefers if we dynamically add it ourselves
#23041 (comment)

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!!

extensions/amp-story/1.0/amp-story-store-service.js Outdated Show resolved Hide resolved
this.element.addEventListener(AmpEvents.DOM_UPDATE, () => {
this.liveStoryManager_.update();
this.initializePages_().then(() => {
this.setDesktopPositionAttributes_(this.activePage_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if UIType.DESKTOP_PANELS

extensions/amp-story/1.0/amp-story.js Show resolved Hide resolved
extensions/amp-story/1.0/live-story-manager.js Outdated Show resolved Hide resolved
this.storeService_.subscribe(StateProperty.PAGE_IDS, () => {
// Make sure the story is laid out before updating.
this.forwardButton_.element.addEventListener(
EventType.STORY_LOADED,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I thought the STORY_LOADED event was dispatched only once, and on the amp-story element

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does work. And I need this because sometimes it was calling onCurrentPageIndexUpdate_ before the story was loaded and it was throwing some errors related to the bookend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something here, could you please explain how this works?
The two things I don't understand are:

  • the EventType.STORY_LOADED event is never triggered on this.forwardButton_.element
  • the EventType.STORY_LOADED event is only triggered once when rendering the story, at the very beginning. You're appending the amp-live-list after this event is dispatched, and it is never triggered again: how can this callback get executed?

* Installs amp-live-list extension automatically.
* Updates pagination buttons in 3-panels view.
* Updates distance attributes on new pages.
* Supports adding a new page in the middle.

w
@Enriqe Enriqe merged commit 71aa512 into ampproject:master Jun 28, 2019
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* Live story polishes.

* Installs amp-live-list extension automatically.
* Updates pagination buttons in 3-panels view.
* Updates distance attributes on new pages.
* Supports adding a new page in the middle.

w

* reviews

* reviews

* use signals to wait for story to be laid out before updating pagination buttons

* fix types

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Pagination buttons aren't updated when a live update comes in
3 participants