Skip to content
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

[TS] Media - Working with audio, video, and unknown media #1673

Closed

Conversation

ignacionr
Copy link
Contributor

@dclaux the last feedback was incorporated, then had to work around merging (the fact that we work on one big file gives git some trouble merging automatically).
I put together a sample at http://media.into.cards/ but still don't like it much. Truth is, I'm out of ideas.

Help, please.
Thanks. Ignacio.//

@ignacionr ignacionr changed the title Feature/media ts 2 [TS] Media - Working with audio, video, and unknown media Jul 2, 2018
@ignacionr
Copy link
Contributor Author

Pulling in the original request #1361

@dclaux
Copy link
Member

dclaux commented Jul 3, 2018

Very sorry for the delay, let me take a look.

@dclaux
Copy link
Member

dclaux commented Jul 3, 2018

@ignacionr I'm going to hold on the actual review of your changes for now, because we need to address the design of the controls first. Thanks for putting the demo site together, that was super helpful.

As is the buttons IMO just don't work well. I don't think we can use the characters you've picked, because they are what they are and can't be styled in CSS. We need to have at least a minimum amount of customization possible in CSS, and we can't have buttons that can only be blue :)

As for the overlay, I think we need to use another way to do the icon in there as well, and there are a couple things that we need to change to make it more "subtle":

  • Remove the black border
  • Remove the drop shadow
  • The button should always be a circle; right now, depending on the size of the poster, it can end up looking like a rectangle with rounded corners

Finally, I think just two buttons doesn't justify reserving an entire pane of empty space below the video.

So here is the design I suggest:

  • Controls are done solely as overlay buttons
  • As the media element is initially rendered, the "Play" overlay button is always displayed
  • If embedded playback isn't supported, clicking the "Play" overlay button opens the stream in another tab (as you have it now)
  • If embedded playback is supported, clicking the "Play" overlay button:
    • Starts playback
    • Changes the "Play" overlay button into a "Pause" overlay button
    • Add a "Reset" overlay button
    • Clicking "Pause" changes "Pause" back into "Play"
  • When the video is playing, the buttons disappear after a couple seconds of the mouse not moving, and are not displayed anyway if the mouse isn't over the media player.

For the overlay buttons, I think we need to use simple inline SVGs for the actual icons. This will allow them to scale properly and as far as I can tell from this page that will make them stylable via CSS.

And in terms of the design, I think it should look something like this:
image

With the play/pause button centered and the rewind button on its left and slightly smaller.

The CSS should make it possible to style the buttons' background color and icon color independently.

That would be for the video player. For the audio player, we should use the same icons, but different CSS styles, since most of the time (and by default) we will need a different background color (transparent white like for the video player won't work.)

Sorry if this is a handful, but we need to make sure these look as good as possible and that hosts can control the appearance of buttons so this Media element fits well within the Adaptive framework.

@ignacionr
Copy link
Contributor Author

Hey @dclaux this is all understood. Thanks for taking the time to look at it, and for the valuable input. Will be updating soon.

@andrewleader
Copy link
Contributor

andrewleader commented Jul 13, 2018

Hey @ignacionr, great work! And thanks for making that demo site! Along with David's feedback, some key points from the original spec that might have not made it into the implementation?

  • The initial overlay play button should ALWAYS be shown, regardless of whether it's video or audio. The media controls (like rewind, etc) only appear once the inline content has started playing.
  • The initial overlay play button should be an image that is obtained from host config (not sure if you're doing that right now). It's simply an image, centered (if host didn't provide one, you should use a default one, SVG sounds great)
  • PDFs aren't supposed to be supported, are they? That's not a video/ or audio/ type, right? Only video and audio are supposed to be supported at this time.

Please review the original spec #196 (it has some updates) to ensure that your implementation is in line with the requirements!

Also, I have some scenario questions about hosts displaying cards that have videos. @ignacionr are you also a host that's displaying cards? Or you're just an author sending cards to Bot Framework or Outlook or something? I fear we might be missing some host requirements, like host ability to pause the video if the user opens a popup dialog, etc.

@dclaux
Copy link
Member

dclaux commented Jul 13, 2018

@andrewleader doing the button as an image in HostConfig is actually not that great a solution. Allowing the styling of the button to be done in CSS is a much better option and satisfies the customization needs. Besides, the design, at least for the HTML media player, calls for more than just the play button to be done as an overlay, and therefore additional properties would have to be added to HostConfig to handle the other button(s).

I suggest we either:

  • Change the design and remove the property from HostConfig, and let hosts customize buttons via other means
  • Or make honoring the HostConfig property optional if the host has a better way to handle customization.

@andrewleader
Copy link
Contributor

HostConfig can be just one of the ways you can customize the button. In HTML it can be...

  • You use the default button, no changes
  • You provide a image in host config for the play button
  • You style the button using CSS

For compatibility and consistency with all the other hosts, the host config needs to be an option, but it doesn't need to be the only way.

@dclaux
Copy link
Member

dclaux commented Jul 13, 2018

I'm saying it doesn't have to be that. In hindsight I dont think I agree with the "need" for the play button to be customizable via HostConfig in the first place. Let's not forget that the role of HostConfig is to provide a way for Host X to emulate Host Y's look and feel, not to be the overall customization story for Adaptive Cards on a single host. In this particular case, I really don't see why Host X would need to render its otherwise completely custom media player with Host Y's choice for the Play overlay button. It seems like a half baked story at best.

So I am suggesting we change the spec.

@dclaux dclaux mentioned this pull request Jul 27, 2018
14 tasks
@dclaux
Copy link
Member

dclaux commented Jul 30, 2018

Hi @ignacionr - I am not sure if you plan to complete this work or not. I think I will pick it up in the coming days if that's OK with you. Let me know.

@ignacionr
Copy link
Contributor Author

@dclaux yes of course. It’s part of the dynamics of this. Happy to keep working together and looking forward to seeing your work.

@khouzam
Copy link

khouzam commented Sep 26, 2018

I think that @dclaux has implemented media and superseded this PR.

@khouzam khouzam closed this Sep 26, 2018
@ignacionr
Copy link
Contributor Author

Agreed.

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.

None yet

4 participants