✨ [Story video] Check if cache response contains audio#36283
✨ [Story video] Check if cache response contains audio#36283mszylkowski merged 27 commits intoampproject:mainfrom
Conversation
|
Hey @gmajoulet, @newmuis! These files were changed: |
|
Update: current status of PR works properly. Tested with page without anything, then page with a video, and page with cached video. |
| const pageHasAudio = | ||
| this.element.hasAttribute('background-audio') || | ||
| this.element.querySelector('amp-audio') || | ||
| hasVideoWithAudio; |
There was a problem hiding this comment.
Nit: Plz put this one first so we don't run the other statements when it's true
There was a problem hiding this comment.
Actually the promise looks like it'd take longer than the querySelectors for inside the page. Should we run the waitForMediaLayout only if the querySelector fails?
gmajoulet
left a comment
There was a problem hiding this comment.
Sorry, actually it's important to switch from end of layoutCallback to end of buildCallback.
layoutCallback for a video on page 10 won't resolve before you navigate to page 8. If this video has audio, the audio icon would only appear when you get to page 8.
|
There's 2 places that need to wait for the videos to build/resolve: when the active page wants to set |
|
Will mark as ready when we merge the icon changes at #37122 since we might not need some of the methods introduced here (eg: the ones that affect the state |
) * Implemented hasAudio from monti * Use promises for hasVideoWithAudio * Added tests * Use waitForPlaybackMediaLayout * Use waitForMediaLayout * Use load signal on visual test * Added return * Remove consolelog * Revert flaky tests * Strict equality check * Added console logs meanwhile * Wait on story for elements that resolve on runtime * Remove unused const * Revert logs * Move wait promise to audio.js * Streamline has videos with audio * Clean comments * Fixed merge errors * Added missing import * Linting * Linting added comma
The video cache can respond in the request whether the video has audio or not, by using the key
"has_audio". We can use that to inform whether we should show the audio UI (equalizer, sound messages, etc).Given how the video cache can resolve after
checkPageHasAudiois called, we need to wait for theamp-videoto finish building before we can tell whether the page has audio, and this works becausebuildCallbackresolves after fetching the cache URLs and applyinghas_audiofield to the video as anoaudioattribute if the video has acache="google"attribute.