-
Notifications
You must be signed in to change notification settings - Fork 37
Make the Mux Playback ID more explicit #115
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,40 +11,69 @@ | |
| import React, { forwardRef } from 'react'; | ||
|
|
||
| // We use and extend Typescript types defined in the MUX player. | ||
|
|
||
| import type MuxPlayerElement from '@mux/mux-player'; | ||
| import type { MuxPlayerProps } from '@mux/mux-player-react'; | ||
|
|
||
| // React MUX player is made available in two flavours: eager and lazy loaded. We | ||
| // choose to use the lazy version to avoid loading the web component uselessly. | ||
| // MUX player lazy version loads internally the eager version using | ||
| // `React.lazy()`. | ||
|
|
||
| import MuxPlayer from '@mux/mux-player-react/lazy'; | ||
|
|
||
| // The core of this component is the `useVideoPlayer` hook: it takes | ||
| // data from DatoCMS GraphQL API and returns props as expected by the | ||
| // `<MuxPlayer />` component. | ||
|
|
||
| import { useVideoPlayer } from '../useVideoPlayer/index.js'; | ||
|
|
||
| type Maybe<T> = T | null; | ||
| type Possibly<T> = Maybe<T> | undefined; | ||
|
|
||
| /** | ||
| * The playback ID is how the player knows which video to play. | ||
| * At least one of these is required. | ||
| * If both are provided, `muxPlaybackId` takes precedence. | ||
| * This is for backward compatibility. | ||
| */ | ||
| type PlaybackIdSources = | ||
| | { muxPlaybackId: Possibly<string> } | ||
| | { playbackId: Possibly<string> }; | ||
|
|
||
| // `Video` represents a fragment of data regarding a video as returned from | ||
| // DatoCMS GraphQL API. | ||
|
|
||
| export type Video = { | ||
| /** | ||
| * Video data as returned by the DatoCMS GraphQL Content Delivery API. | ||
| * | ||
| * You MUST provide `muxPlaybackId` (which is a field in your GraphQL query). The `streamingUrl` property is not used here. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's (I mean, I know what it is, but we should better explain it in here, or it will be even more confusing, since nowhere in this code or in the docs there's any mention of this property) |
||
| * | ||
| * As long as you provide `muxPlaybackId`, you can safely ignore the `playbackId` param. If you accidentally provide both, `muxPlaybackId` takes precedence. | ||
| * | ||
| * @property muxPlaybackId - Mux playback ID | ||
| * @property playbackId - (Optional, fallback only) Alias for the `muxPlaybackId` param, for backward compatibility | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So far, We should drop support for |
||
| * @property title - Title attribute (`title`) for the video. | ||
| * @property height - The height of the video in pixels. | ||
| * @property width - The width of the video in pixels. | ||
| * @property blurUpThumb - A `data:` URI containing a blurhash placeholder image. | ||
| * @property [k: string] - Any additional properties are accepted but not used | ||
| * by the renderer. | ||
| * | ||
| * @example | ||
| * ```ts | ||
| * const video: Video = { | ||
| * muxPlaybackId: "abcdef123456", | ||
| * title: "Example video", | ||
| * width: 1920, | ||
| * height: 1080, | ||
| * blurUpThumb: "data:image/png;base64,..." | ||
| * }; | ||
| * ``` | ||
| */ | ||
| export type Video = PlaybackIdSources & { | ||
| /** Title attribute (`title`) for the video */ | ||
| title?: Possibly<string>; | ||
| /** The height of the video */ | ||
| height?: Possibly<number>; | ||
| /** The width of the video */ | ||
| width?: Possibly<number>; | ||
| /** The MUX playbaack ID */ | ||
| muxPlaybackId?: Possibly<string>; | ||
| /** The MUX playbaack ID */ | ||
| playbackId?: Possibly<string>; | ||
| /** A data: URI containing a blurhash for the video */ | ||
| blurUpThumb?: Possibly<string>; | ||
| /** Other data can be passed, but they have no effect on rendering the player */ | ||
|
|
@@ -70,7 +99,7 @@ export const VideoPlayer: ( | |
| VideoPlayerProps | ||
| >((props, ref) => { | ||
| const { | ||
| data = {}, | ||
| data = { muxPlaybackId: undefined }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this for? 😕 |
||
| disableCookies = true, | ||
| disableTracking = true, | ||
| preload = 'metadata', | ||
|
|
||
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.
Possiblymeans it can be null/undefined, so I think we have achieved nothing here 😆Didn't we want to make one of them required?
Also, you've missed the JSDoc for the two properties.