amp-story initial load tweaks #12842
Conversation
…tes progress bar through PAGE_PROGRESS events
…age updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899.
| @@ -1282,7 +1337,6 @@ export class AmpStory extends AMP.BaseElement { | |||
| if (this.bookend_.isActive()) { | |||
| // Dispaching event instead of calling method directly so that all | |||
| // listeners can respond. | |||
| dispatch(this.element, EventType.CLOSE_BOOKEND); | |||
There was a problem hiding this comment.
I don't think you should remove this :)
There was a problem hiding this comment.
How did that happen?!
There was a problem hiding this comment.
My guess is cut/paste for dispatch? ;)
| @@ -195,8 +202,15 @@ export class AmpStoryPage extends AMP.BaseElement { | |||
| } | |||
|
|
|||
|
|
|||
| /** @return {!Promise} */ | |||
| waitUntilLoaded() { | |||
|
|
||
| /** @private */ | ||
| markStoryAsLoaded_() { | ||
| dispatch(this.element, EventType.STORY_LOADED, true); |
There was a problem hiding this comment.
Is there a listener for this?
There was a problem hiding this comment.
Not within the runtime; it's meant for embedding pages to listen to (e.g. test environments, tools, etc)
| @@ -502,12 +520,49 @@ export class AmpStory extends AMP.BaseElement { | |||
| } | |||
|
|
|||
|
|
|||
| /** @private */ | |||
| waitForInitialLoad_() { | |||
There was a problem hiding this comment.
Could be simpler. Can be one method. Timeout can be implied in parameter.
whenPagesLoaded_(pagesToWaitFor, timeoutMs = 0) {
const pagesToWaitFor = this.isDesktop_() ?
[this.pages_[0], this.pages_[1]] :
[this.pages_[0]];
const loadPromise = Promise.all(pagesToWaitFor.map(page => page.whenLoaded());
return this.timer_.timeoutPromise(timeoutMs, loadPromise);
}| @@ -91,6 +91,19 @@ const DESKTOP_HEIGHT_THRESHOLD = 550; | |||
| /** @private @const {number} */ | |||
| const MIN_SWIPE_FOR_HINT_OVERLAY_PX = 50; | |||
|
|
|||
| /** | |||
| * The duration of time (in milliseconds) to wait for a page to be loaded. | |||
There was a problem hiding this comment.
...for a page to be loaded before story becomes visible.
css/amp.css
Outdated
| @@ -803,6 +803,18 @@ amp-accordion > section[expanded] > :last-child { | |||
| width: 100%; | |||
| } | |||
|
|
|||
| amp-story[standalone] { | |||
| background-color: #fff !important; | |||
There was a problem hiding this comment.
What if the author wants to override this?
There was a problem hiding this comment.
This is rarely ever shown. This is the background of the story itself, but not the pages, and all of our experiences show the background of the page (which the publisher can override). This should only show up while the story is loading.
There was a problem hiding this comment.
Right, I missed that. Thanks.
…errors to ensure all elements have a chance to load.
|
@alanorozco PTAL when you get a chance |
| // promises to complete, even if one has failed. We do nothing with the | ||
| // error, as the resource itself and/or code that loads it should handle | ||
| // the error. | ||
| mediaEl.addEventListener('error', resolve, true /* useCapture */); |
There was a problem hiding this comment.
Does this change what gets output to the console in case of an error?
| /** @private */ | ||
| markPageAsLoaded_() { | ||
| this.element.classList.add(PAGE_LOADED_CLASS_NAME); | ||
| this.mutateElement(() => { | ||
| dispatch(this.element, EventType.PAGE_LOADED, true); |
There was a problem hiding this comment.
Perhaps dispatch event outside mutate callback.
* Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events * Revert "Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899. * Modify initial loading styles and signals. * Fix lint and types * Change loading logic to depend on canplay events for media. Suppress errors to ensure all elements have a chance to load. * Refactor page load utility functions in amp-story * Fix lint * Move dispatch out of vsync * Add whitespace to trigger Travis re-run * Remove unnecessary whitespace * Rename loadPromise vars * Fix line length
* Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events * Revert "Change progress bar to not handle active page, since active page updates progress bar through PAGE_PROGRESS events" This reverts commit 0500899. * Modify initial loading styles and signals. * Fix lint and types * Change loading logic to depend on canplay events for media. Suppress errors to ensure all elements have a chance to load. * Refactor page load utility functions in amp-story * Fix lint * Move dispatch out of vsync * Add whitespace to trigger Travis re-run * Remove unnecessary whitespace * Rename loadPromise vars * Fix line length
This fixes the initial styling of standalone
amp-storyelements by showing a blank white screen until the story is "loaded" (all the media assets on the initially-visible pages have loaded at least one frame)./cc @ithinkihaveacat This PR adds an event (
ampstory:load) that is triggered when the story is deemed loaded, as well as a CSS class (.i-amphtml-story-loaded) that gets added to theamp-storytag when it is loaded. You can also continue to use the page-level CSS class (.i-amphtml-story-page-loaded), and there is also an event (ampstory:pageload) that is triggered when a page is loaded./cc @hongwei1990 @christianbersch @flaviori