feat(Embed|Video): add components

[stuartlong]

API Proposal

The goal of this proposal is to provide components similar to Image that work with video sources.

1. The Video component is a very basic wrapper around the
2. The Embed component is a more complex one that displays either an embedded video (GIF) or an embedded iFrame

Video Component

Essentially just the normal

 <Video
      src="https://raw.githubusercontent.com/bower-media-samples/big-buck-bunny-480p-30s/master/video.mp4"
      poster="https://raw.githubusercontent.com/bower-media-samples/big-buck-bunny-480p-30s/master/poster.jpg"
      variables={{ width: '600px' }}
  />

Problems

1. Video is played through default HTML5 video controls. These controls are accessible, but only via the tab key. Ideally, a user would tab to the video and then use the arrow keys to navigate through the video controls, but not sure how to get this to work within the stardust framework

2. Muted attribute doesn't work. I can't get the muted attribtue to appear on the outputted html:

     <Video muted />

   and

   <Video muted="true" />

   both result in

Embed Component

This component displays either an Image or Video comppnent based on whether the GIF is currently playing. We do this swap out due to memory issues with the

If a video isn't provided, an iFrame can be provided to embed instead

The component also adds key handling for space / enter when focused on the gif to play/pause it.

 <Emned
      video="https://raw.githubusercontent.com/bower-media-samples/big-buck-bunny-480p-5s/master/video.mp4"
      poster="https://raw.githubusercontent.com/bower-media-samples/big-buck-bunny-480p-5s/master/poster.jpg"
      variables={{ width: '600px' }}
      active={true}
  />

Problems

1. Displaying SVG as ::after content. I display the play/pause button as an ::after pseudoelement so it doesn't take up any slots or room in the DOM. However, I couldn't find a way to use the stored icons SVG and instead had to hard code the svg within the styles file itself.

TODO

• Conformance test
• Minimal doc site example
• Stardust base theme
• Teams Light theme
• Teams Dark theme
• Teams Contrast theme
• Confirm RTL usage
• W3 accessibility check
• Stardust accessibility check
• Update glossary props table
• Update the CHANGELOG.md

[layershifter]

@stuartlong great work, however from components perspective I want to add some separation there.

Video

We can definitely add this component and then iterate on it to apply styling to controls and add more props.

VideoGif

Instead of it, I propose to add Embed component that will allow to handle not only videos, but even iframes. See for reference, https://react.semantic-ui.com/modules/embed/

1. Rename to VideoGif to Embed
2. Add video shorthand slot, add iframe shorthand slot and use Box component

render() {
  const { iframe, video } = this.props

  return (
    <>
      {Video.create(video)}
      {Box.create(iframe, { defaultProps: { as: 'iframe' } })}
     </>
   )
}

3. As we are trying to match SUIR API, I suggest to rename poster prop to placeholder and use shorthand for Image:

render() {
  const { placeholder } = this.props

  return (
    <>
      {Image.create(placeholder)}
     </>
   )
}

[layershifter]

I suggest to drop accessibility part for initial implementation at all and introduce it later with sync with our accessibility team. What do you think?

[stuartlong]

Sounds good for Video. For Video Gif, I'd like to keep in the key actions so that space/enter on GIFs still play/pause them. But if there's some issue there too we can take that up after the accessibility sync.

[codecov]

Codecov Report

Merging #1108 into master will increase coverage by 0.05%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #1108      +/-   ##
==========================================
+ Coverage   82.47%   82.53%   +0.05%     
==========================================
  Files         743      750       +7     
  Lines        8761     8846      +85     
  Branches     1236     1178      -58     
==========================================
+ Hits         7226     7301      +75     
- Misses       1520     1530      +10     
  Partials       15       15

Impacted Files	Coverage Δ
packages/react/src/themes/teams/index.ts	92% <ø> (ø)	⬆️
...ges/react/src/themes/base/components/Icon/index.ts	100% <ø> (ø)	⬆️
...rc/themes/teams/components/Embed/embedVariables.ts	100% <100%> (ø)
packages/react/src/index.ts	100% <100%> (ø)	⬆️
...kages/react/src/themes/teams/componentVariables.ts	100% <100%> (ø)	⬆️
...lib/accessibility/Behaviors/Embed/embedBehavior.ts	100% <100%> (ø)
packages/react/src/lib/accessibility/index.ts	100% <100%> (ø)	⬆️
packages/react/src/themes/teams/componentStyles.ts	100% <100%> (ø)	⬆️
...rc/themes/teams/components/Video/videoVariables.ts	100% <100%> (ø)
...t/src/themes/teams/components/Video/videoStyles.ts	33.33% <33.33%> (ø)
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d37c616...dae16b9. Read the comment docs.

[mnajdova]

Let's add onActiveChanged prop for this component, and invoke it when we change the active prop, as it will be required when we want to use it in controlled mode (see for example the onOpenChanged in the Popup component)

[layershifter]

Implemented as onActiveChanged, however I want address an inconsistency there separately because Popup has onOpenChange (without d on end).

[mnajdova]

I would expect the d in the end, but agree, let's address in a separate PR, to avoid unnecessary breaking changes.

[mnajdova]

No need for this, as the defaultBehavior will be applied if nothing is provided here.

[layershifter]

We have currently an assertion for this, going to address it in a separate PR.

[mnajdova]

This is where we would invoke the onActiveChanged method.

[bmdalex]

great work, no more comments on top of the ones from existing reviewers, pls address them before merge

[mnajdova]

Please let's use colors values in the variables, so that we may override those in different themes.

[mnajdova]

One last comment. LGTM 👍