-
Notifications
You must be signed in to change notification settings - Fork 55
feat(Embed|Video): add components #1108
feat(Embed|Video): add components #1108
Conversation
Adds the Video and VideoGif components along with documentation, styling, and tests. Video is a basic wrapper around the base video HTML tag, however VideoGif is slightly more complex. It swaps out between a Video and Image component based on if the GIF is playing or not and adds a hover icon the user can click to start/stop the GIF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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/
- Rename to
VideoGif
toEmbed
- Add
video
shorthand slot, addiframe
shorthand slot and useBox
component
render() {
const { iframe, video } = this.props
return (
<>
{Video.create(video)}
{Box.create(iframe, { defaultProps: { as: 'iframe' } })}
</>
)
}
- As we are trying to match SUIR API, I suggest to rename
poster
prop toplaceholder
and use shorthand forImage
:
render() {
const { placeholder } = this.props
return (
<>
{Image.create(placeholder)}
</>
)
}
|
||
/** | ||
* An video is a graphicical and audio representation of something. | ||
* @accessibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/react/src/themes/teams/components/Icon/svg/icons/pause.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Icon/svg/icons/pause.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Video/videoGifStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedVariables.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/embed/embedStyles.ts
Outdated
Show resolved
Hide resolved
packages/react/src/lib/accessibility/Behaviors/Embed/embedBehavior.ts
Outdated
Show resolved
Hide resolved
…at/stuartlo/video # Conflicts: # packages/react/src/themes/teams/components/Icon/svg/icons/pause.tsx # packages/react/src/themes/teams/components/Icon/svg/icons/play.tsx
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
} | ||
|
||
export interface EmbedState { | ||
active: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented as onActiveChanged
, however I want address an inconsistency there separately because Popup
has onOpenChange
(without d on end).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect the d in the end, but agree, let's address in a separate PR, to avoid unnecessary breaking changes.
|
||
static defaultProps = { | ||
as: 'video', | ||
accessibility: defaultBehavior as Accessibility, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this, as the defaultBehavior
will be applied if nothing is provided here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have currently an assertion for this, going to address it in a separate PR.
e.stopPropagation() | ||
e.preventDefault() | ||
|
||
this.trySetState({ active: !this.state.active }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where we would invoke the onActiveChanged
method.
packages/react/src/themes/teams/components/Embed/embedVariables.ts
Outdated
Show resolved
Hide resolved
packages/react/src/themes/teams/components/Video/videoVariables.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work, no more comments on top of the ones from existing reviewers, pls address them before merge
…at/stuartlo/video
…at/stuartlo/video # Conflicts: # CHANGELOG.md
} | ||
}, | ||
control: ({ props: p, variables: v }): ICSSInJSStyle => ({ | ||
background: `0 no-repeat rgba(0,0,0,.25)`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let's use colors values in the variables, so that we may override those in different themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last comment. LGTM 👍
API Proposal
The goal of this proposal is to provide components similar to Image that work with video sources.
Video Component
Essentially just the normal
Problems
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
Muted attribute doesn't work. I can't get the muted attribtue to appear on the outputted html:
and
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.
Problems
::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