-
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
Conversation
✅ Deploy Preview for cld-video-player ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for cld-vp-esm-pages ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
LGTM 💪
playerWithCustomProfile.source('samples/cld-sample-video'); | ||
}, false); | ||
</code> | ||
</pre> | ||
</div> | ||
|
||
</body> |
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
src/player.js
Outdated
} | ||
const urlPrefix = unsigned_url_prefix( | ||
null, | ||
initOptions.cloudName || initOptions.cloud_name, |
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.
initOptions.cloudName || initOptions.cloud_name, | |
initOptions.cloudName ?? initOptions.cloud_name, |
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.
👍
src/player.js
Outdated
|
||
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 comment
The 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
return await fetch(profileUrl, { method: 'GET' }).then(res => res.json()); | |
return fetch(profileUrl, { method: 'GET' }).then(res => res.json()); |
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.
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 urlPrefix = unsigned_url_prefix( | ||
null, | ||
initOptions.cloudName ?? initOptions.cloud_name, | ||
initOptions.private_cdn, | ||
initOptions.cdn_subdomain, | ||
initOptions.secure_cdn_subdomain, | ||
initOptions.cname, | ||
initOptions.secure, | ||
initOptions.secure_distribution, | ||
); |
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.
@jakub-roch instead of creating that list can't you use CLOUDINARY_CONFIG_PARAM
, like in getConfig() ?
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.
videoPlayerWithProfile