Skip to content

Refactor Video assets to allow sound, poster support to replayable videos, text fixes. #406

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

Conversation

dobromir-hristov
Copy link
Contributor

Bug/issue #, if applicable: 97715398

Summary

  1. Refactors the Video component to allow rendering videos with sound.
  2. Properly pass posterVariants to VideoAsset.vue from ReplayableVideoAsset.vue. This was a bug left from before.
  3. Make sure ReplayableVideoAsset.vue shows the proper text in it's "Play" butotn, if its not autoplayed yet.

Dependencies

NA

Testing

This PR can be best tested with

Steps:

  1. Assert Videos can be rendered with sound ON.
  2. Assert that videos with Sound and Autoplay, show a "Play" button.
  3. Assert that the "Play" button changes to "Replay", upon video completion.
  4. Assert that in Safari, it shows a "Play" button, even if autoplay is ON with sound.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

Properly pass posterVariants to VideoAsset.vue from ReplayableVideoAsset.vue.
Make sure ReplayableVideoAsset.vue shows the proper text, if its not autoplayed yet.
Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

Code changes generally look good to me.

Could you sync up with me offline with an example video that you used to test this? I'm curious about the "Play" button state, but I suspect there's no issue.

Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

I realized what I was confused about since the "Play" state is only something that can be seen with the other PR for introducing the new Video support within content blocks, although it's not technically a dependency here.

@dobromir-hristov
Copy link
Contributor Author

@swift-ci test

@dobromir-hristov dobromir-hristov merged commit 7cf76ef into swiftlang:main Sep 12, 2022
@dobromir-hristov dobromir-hristov deleted the dhristov/r97715398-add-sound-to-replayable-videos branch September 12, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants