-
Notifications
You must be signed in to change notification settings - Fork 357
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
Video: add objectFill
as a prop
#1227
Video: add objectFill
as a prop
#1227
Conversation
@@ -11,6 +11,8 @@ type Source = | |||
| string | |||
| Array<{| type: 'video/m3u8' | 'video/mp4' | 'video/ogg', src: string |}>; | |||
|
|||
type ObjectFit = 'fill' | 'contain' | 'cover' | 'none' | 'scale-down'; |
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.
MDN for reference: https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit
packages/gestalt/src/Video.js
Outdated
@@ -549,6 +551,8 @@ export default class Video extends PureComponent<Props, State> { | |||
onSeeked={this.handleSeek} | |||
onTimeUpdate={this.handleTimeUpdate} | |||
onProgress={this.handleProgress} | |||
// $FlowIssue facebook/flow#8448 |
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.
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.
@valbooth28 thanks for the PR! Made a few updates:
- Remove the $FlowIssue - see Common spread pattern leads to "exponentially large number of cases" errors facebook/flow#8186 (comment)
- Updated the docs to include the
objectFit
prop - Added tests to ensure objectFit gets rendered
Also know that with every flow issue, we require the exact exception, so instead of something like // $FlowFixMe
, we should write // $FlowFixMe[prop-missing]
. See gajus/eslint-plugin-flowtype#453 for the request to make this an ESLint rule
Deploy preview for gestalt ready! Built with commit d52e8a9 |
aad1e37
to
d52e8a9
Compare
@@ -549,7 +552,10 @@ export default class Video extends PureComponent<Props, State> { | |||
onSeeked={this.handleSeek} | |||
onTimeUpdate={this.handleTimeUpdate} | |||
onProgress={this.handleProgress} | |||
{...(crossOrigin ? { crossOrigin } : null)} | |||
{...(objectFit ? { style: { 'object-fit': objectFit } } : null)} | |||
{...((crossOrigin ? { crossOrigin } : { ...null }): {| |
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.
@AlbertCarreras / @jennyscript fyi - a pattern we might see more often when we spread on a component. It's pretty verbose but better than adding a $FlowIssue
/ $FlowFixMe
: facebook/flow#8186 (comment)
The { ...null }
seems odd but it's what we have to do to make it work with exact types
objectFill
as a propTypeobjectFill
as a prop
Background
When Story Pins started using the Gestalt
Video
component for MP4s (experiment gated), employees noticed that the "poster", or placeholder while the video loads, was being displayed distorted.Repro
Gif reproduction:
Screenshot for emphasis.
Changes
Setting the style as 'object-fill: contain
seems to fix the issue, so this PR adds the specific
object-fill` part of the style as an optional prop.Test Plan
Internal e2e testing confirms this will be a fix, and you can locally observe the style is passed correctly: