-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add url template for video player profiles #696
Changes from 5 commits
3e8d981
a6b3f63
1139d9f
d5deaa2
a8545b3
6b005aa
a7a5a26
4233c58
d833625
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,23 +1,32 @@ | ||||||
import VideoPlayer from './video-player'; | ||||||
import { defaultProfiles } from './config/profiles'; | ||||||
import { isRawUrl } from './plugins/cloudinary/common'; | ||||||
import { unsigned_url_prefix } from '@cloudinary/url-gen/backwards/utils/unsigned_url_prefix'; | ||||||
|
||||||
export const getProfile = async (cloudName, profile) => { | ||||||
export const getProfile = async (profile, initOptions) => { | ||||||
if (Object.keys(defaultProfiles).includes(profile)) { | ||||||
return defaultProfiles[profile]; | ||||||
} | ||||||
|
||||||
if (isRawUrl(profile)) { | ||||||
return await fetch(profile, { method: 'GET' }).then(res => res.json()); | ||||||
} | ||||||
const urlPrefix = unsigned_url_prefix( | ||||||
null, | ||||||
initOptions.cloudName || initOptions.cloud_name, | ||||||
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.
Suggested change
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. 👍 |
||||||
initOptions.private_cdn, | ||||||
initOptions.cdn_subdomain, | ||||||
initOptions.secure_cdn_subdomain, | ||||||
initOptions.cname, | ||||||
initOptions.secure, | ||||||
initOptions.secure_distribution, | ||||||
); | ||||||
|
||||||
throw new Error('Custom profiles will be supported soon, please use one of default profiles: "cldDefault", "cldLooping" or "cldAdaptiveStream"'); | ||||||
const profileUrl = isRawUrl(profile) ? profile : `${urlPrefix}/_applet_/video_service/video_player_profiles/${profile}.json`; | ||||||
return await fetch(profileUrl, { method: 'GET' }).then(res => res.json()); | ||||||
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. I think the await here and the async in the function declaration are both redundant, but especially here it is really not needed
Suggested change
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. changed, I would like to keep entire method as async just in case - so w need to have async declaration on the top but there await can be removed |
||||||
}; | ||||||
|
||||||
const player = async (elem, initOptions, ready) => { | ||||||
const { profile, ...otherInitOptions } = initOptions; | ||||||
try { | ||||||
const profileOptions = profile ? await getProfile(otherInitOptions.cloud_name, profile) : {}; | ||||||
const profileOptions = profile ? await getProfile(profile, otherInitOptions) : {}; | ||||||
const options = Object.assign({}, profileOptions.playerOptions, otherInitOptions); | ||||||
const videoPlayer = new VideoPlayer(elem, options, ready); | ||||||
|
||||||
|
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 adding another example with a custom profile and overriding configuration code to make sure it works as expected (It could be same page but two different players and i think this is even more favourable)
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.
good point, added another example with overrides