Skip to content
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

CldVideoPlayer config prop #196

Closed
colbyfayock opened this issue Mar 22, 2024 · 2 comments · Fixed by #200
Closed

CldVideoPlayer config prop #196

colbyfayock opened this issue Mar 22, 2024 · 2 comments · Fixed by #200
Labels
enhancement New feature or request

Comments

@colbyfayock
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

Allow CldVideoPlayer to take a config prop, much like CldImage, which can control advanced Cloudinary settings

This would then ideally inherit module-level configuration that's being set up via: #195

Describe the solution you'd like

The Next.js implementation currently passes through any valid option that's defined in the source types from cloudinary-util/types

https://github.com/cloudinary-community/next-cloudinary/blob/main/next-cloudinary/src/components/CldVideoPlayer/CldVideoPlayer.tsx#L44

https://github.com/colbyfayock/cloudinary-util/blob/main/packages/types/src/types/cloudinary-video-player.d.ts

It doesnt look liek the work was done yet to use the types from the new package: #186

By adding the same thing, this would allow any of the configuration options to "just work"

@colbyfayock colbyfayock added the enhancement New feature or request label Mar 22, 2024
@Baroshem
Copy link
Collaborator

Quick question about global config. Are you sure that we want to pass the config from global configuration to the CldVideoPlayer as well? The global config uses the CloudinaryConfigurationOptions that is the same for the constructCloudinaryUrl metho and I am not sure if it is the same as the configuration for the video player. If it is the same, then we are safe to go.

Baroshem added a commit that referenced this issue Mar 23, 2024
@Baroshem Baroshem linked a pull request Mar 23, 2024 that will close this issue
6 tasks
@colbyfayock
Copy link
Collaborator Author

colbyfayock commented Mar 24, 2024

good question about the config, but we need to normalize on something and i think it makes sense to normalize on the config options as defined in ConfigOptions of URL Loader already, which is the same shape that the underlaying URL Gen library uses (official Cloudinary SDK)

<CldImage config should be that same shape, so i'm thinking <CldVideoPlayer config should as well, but then we would need to apply the properties differently than what we do with the image due to the nature of the configuration options

https://github.com/colbyfayock/cloudinary-util/blob/main/packages/url-loader/src/types/config.ts
https://github.com/cloudinary/js-url-gen/blob/master/src/config/interfaces/Config/ICloudinaryAssetConfigurations.ts

@Baroshem Baroshem linked a pull request Mar 24, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants