Fallback "Play video" message when the story video failed to play. #19696
Conversation
gmajoulet
force-pushed
the
play_message_fallback
branch
from
3d56144
to
55dde9c
Compare
| */ | ||
| const buildPlayMessageElement = element => | ||
| htmlFor(element)` | ||
| <button role="button" class="i-amphtml-story-page-play-button"> |
There was a problem hiding this comment.
Should we at least have the reset class on this? Arguably, the spinner and this could go in a shadow tree
There was a problem hiding this comment.
I added the system reset. Rendering some shadow DOM in the amp-story-page would require a bit more work, in term of positioning, being able to re-use it for the spinner, and especially having a separate file for the CSS, etc. Lmk if you want me to work on that, it shouldn't be too crazy.
| this.playMessageEl_.addEventListener('click', () => { | ||
| this.togglePlayMessage_(false); | ||
| this.mediaPoolPromise_ | ||
| .then(mediaPool => mediaPool.blessAll()) |
There was a problem hiding this comment.
How does this perform on iOS? In theory, I think all we need is the play call; the bless is just nice to have.
There was a problem hiding this comment.
I can show you tomorrow on the iPhone 7 or 5 :)
I think it's a product question, because we might need to bless the video for the auto-advance case. In the demo I posted, if we remove the bless and wait for the story to auto advance, we'd have to tap the button on the second page to play the video.
However, if we bless it, it's going to play if we tap OR wait. I opted for this since it provides a more consistent experience. It's fine on the iPhone 7, but if you think it's too much of a performance hit, we can remove it.
We could play the videos and then bless everything, but I'm afraid the bless actions would freeze the playing video. :((
There was a problem hiding this comment.
+1. It's better to bless if possible, I just want to make sure the performance is not a blocker.
| @@ -643,3 +643,47 @@ amp-story .amp-video-eq, | |||
| 50% { transform: rotate(5deg) } | |||
| to { transform: rotate(-130deg) } | |||
| } | |||
|
|
|||
| .i-amphtml-story-page-play-button { | |||
There was a problem hiding this comment.
Is there no way to share some of this with the sound icon message?
| @@ -54,6 +54,9 @@ export default /** @const {!LocalizedStringBundleDef} */ ({ | |||
| [LocalizedStringId.AMP_STORY_DOMAIN_DIALOG_HEADING_LINK]: { | |||
| string: 'More about AMP results', | |||
| }, | |||
| [LocalizedStringId.AMP_STORY_PAGE_PLAY_VIDEO]: { | |||
| string: 'Play video', | |||
There was a problem hiding this comment.
I almost wonder if we're better off leaving this blank. This is used if we don't support the user's language, where we fall back (as much as possible) to iconography
There was a problem hiding this comment.
Done. It still works OK: screenshot
gmajoulet
force-pushed
the
play_message_fallback
branch
from
55dde9c
to
03518ed
Compare
| @@ -54,6 +54,9 @@ export default /** @const {!LocalizedStringBundleDef} */ ({ | |||
| [LocalizedStringId.AMP_STORY_DOMAIN_DIALOG_HEADING_LINK]: { | |||
| string: 'More about AMP results', | |||
| }, | |||
| [LocalizedStringId.AMP_STORY_PAGE_PLAY_VIDEO]: { | |||
| string: 'Play video', | |||
There was a problem hiding this comment.
Done. It still works OK: screenshot
| this.playMessageEl_.addEventListener('click', () => { | ||
| this.togglePlayMessage_(false); | ||
| this.mediaPoolPromise_ | ||
| .then(mediaPool => mediaPool.blessAll()) |
There was a problem hiding this comment.
I can show you tomorrow on the iPhone 7 or 5 :)
I think it's a product question, because we might need to bless the video for the auto-advance case. In the demo I posted, if we remove the bless and wait for the story to auto advance, we'd have to tap the button on the second page to play the video.
However, if we bless it, it's going to play if we tap OR wait. I opted for this since it provides a more consistent experience. It's fine on the iPhone 7, but if you think it's too much of a performance hit, we can remove it.
We could play the videos and then bless everything, but I'm afraid the bless actions would freeze the playing video. :((
| */ | ||
| const buildPlayMessageElement = element => | ||
| htmlFor(element)` | ||
| <button role="button" class="i-amphtml-story-page-play-button"> |
There was a problem hiding this comment.
I added the system reset. Rendering some shadow DOM in the amp-story-page would require a bit more work, in term of positioning, being able to re-use it for the spinner, and especially having a separate file for the CSS, etc. Lmk if you want me to work on that, it shouldn't be too crazy.
gmajoulet
force-pushed
the
play_message_fallback
branch
from
03518ed
to
bbf90a9
Compare
|
Just synced this branch to make Travis/Percy green :)) |
…mpproject#19696) * Fallback Play video message for when auto play fails. * Display message animation. * Wrapping in mutate + i18n. * Reviews.
Story videos might fail to auto play for various reasons (data saver mode, low battery mode, probably others...). This PR displays a "Play video" message to capture a user intent and bless / play the videos.
Temporary demo: it has a video on the first page, and it auto advances to another video. If you click the "Play video" message on the first page, the video on the second page should have been "blessed" and auto play too.
Fixes #12786