[amp-story-player] Player refactoring ♻️ #32115
Conversation
Enriqe
force-pushed
the
player-refactor
branch
from
c4eeb72 to
533d6a6
Compare
Enriqe
force-pushed
the
player-refactor
branch
from
d0cb6fd to
9dc06bf
Compare
There was a problem hiding this comment.
Comments for followup PRs:
-
the most important one: we should have only one single code path to update all story positions/load/prerender/etc. One unique method like "update" or "render" or whatever you like. Right now you have different code paths to update stories: from
prerenderStories, fromadd, etc. We should have only one "update" that updates everything: get distance map, set positions, add/remove from DOM, set visibilityStates, loads N+1 when N is ready -
setSrc_ that handles load of followup stories is a bit weird. That should be fixed by/with the comment above.
-
buildStories_ that can append to DOM, that should be fixed by/with the comment above. This should build the StoryDefs and that's it, no side effects.
-
updateStoryPosition should be the only code path, we should remove "updateCurrent" "updatePrevious" etc (or keep them as private internals, only called from updateStoryPosition). Each StoryDef knows its previous distance/position, so you know if it needs to be removed/added from the DOM, repositioned, or any operations
| idx === 0 /** hasPriorityLoading */ | ||
| ).then(() => this.prerenderFirstStoryDeferred_.resolve()); | ||
| } | ||
| this.stories_.forEach((story) => { |
There was a problem hiding this comment.
Should we really loop on all Stories here? I'd like us to simplify the code further, and have one single code path to position/prerender/load stories based on their distance.
Right now it can happen from add or from prerenderStories or other methods. Would be cool if a single method was handling everything. (in a followup)
| @@ -843,14 +768,14 @@ export class AmpStoryPlayer { | |||
|
|
|||
| /** Sends a message muting the current story. */ | |||
| mute() { | |||
| const {iframeIdx} = this.stories_[this.currentIdx_]; | |||
| this.updateMutedState_(iframeIdx, true); | |||
| const story = this.stories_[this.currentIdx_]; | |||
There was a problem hiding this comment.
Pretty nitty but if it's easy I think it'd be cool to pass the StoryDef as a parameter. Right now for ex, you couldn't mute the current story and unmute the next one. I know it wouldn't work on the web anyway but it would be nice to support it eventually, and to have method signature consistency.
There was a problem hiding this comment.
Since this is a public API it would mean a breaking change :/
Enriqe
force-pushed
the
player-refactor
branch
from
7165c5f to
bcd07db
Compare
* fix tests * delete iframepool and refactor methods to only use storyDef * jsdoc * jsdoc * cleanups * delete obsolete test * jsdoc
* fix tests * delete iframepool and refactor methods to only use storyDef * jsdoc * jsdoc * cleanups * delete obsolete test * jsdoc
* fix tests * delete iframepool and refactor methods to only use storyDef * jsdoc * jsdoc * cleanups * delete obsolete test * jsdoc
* fix tests * delete iframepool and refactor methods to only use storyDef * jsdoc * jsdoc * cleanups * delete obsolete test * jsdoc
StoryDef.messagingPromiseto be a property of each story.distanceto know when to append a story to the DOM.Deltas:
Followup:
WeakRefso that they can be GC when needed.