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

rawTransformations needed on <CldVideoPlayer /> like on useCldVideoUrl #239

Closed
billnbell3 opened this issue Sep 25, 2024 · 16 comments · Fixed by #246
Closed

rawTransformations needed on <CldVideoPlayer /> like on useCldVideoUrl #239

billnbell3 opened this issue Sep 25, 2024 · 16 comments · Fixed by #246
Labels
enhancement New feature or request

Comments

@billnbell3
Copy link

It would be awesome if we can send rawTransformations in CldVideoPlayer like we can in useCldVideoUrl!

The transformations property just appends t_ and does not do what we want.

See below for how we got it to work without CldVideoPlayer:

<template>
  <video
    ref="videoPlayerRef"
    v-if="cldVideo && isCldVideo"
    :src="getVideoUrl()"
    :loop="isCldVideoLoop"
    autoPlay="always"
    :controls="isCldVideoSound"
    :quality="quality"
    :muted="!isCldVideoSound"
  />
  <!-- <CldVideoPlayer
    ref="videoPlayerRef"
    v-if="cldVideo && isCldVideo"
    :src="src2"
    :loop="isCldVideoLoop"
    autoPlay="always"
    :playsinline="true"
    :controls="isCldVideoSound"
    :muted="!isCldVideoSound"
    :quality="quality"
    :fetch-format="format"
    controlsList="nodownload"
    :config="{ autoplay: true }"
  /> -->
</template>

<script>
  export default {
    props: {
      //cloudinary video props
      cldVideo: {
        type: Object,
        default: null,
      },
      isCldVideoQAuto: {
        type: Boolean,
        default: true,
      },
      isCldVideoFAuto: {
        type: Boolean,
        default: true,
      },
      isCldVideoSound: {
        type: Boolean,
        default: false,
      },
      isCldVideoLoop: {
        type: Boolean,
        default: true,
      },
      isCldVideoFadeEffect: {
        type: Boolean,
        default: false,
      },
      isCldVideo: {
        type: Boolean,
        default: false,
      },
    },
    data() {
      return {
        quality: String,
        format: String,
        videoId: String,
      };
    },
    created() {
      //mapping width and height from selected dimensions option
      if (this.isCldVideo) {
        this.quality = this.isCldVideoQAuto ? 'auto' : '';
        this.format = this.isCldVideoFAuto ? 'auto' : '';
        this.videoId = this.cldVideo?.public_id.replace(/ /g, '%20');
      }
    },
    methods: {
      getVideoUrl() {
        const { url } = useCldVideoUrl({
          options: {
            src: this.videoId,
            quality: this.quality,
            controls: this.isCldVideoSound,
            autoplay: true,
            controlsList: 'nodownload',
            fetchFormat: this.format,
            rawTransformations: this.isCldVideoFadeEffect
              ? 'e_fade:2000/e_fade:-2000'
              : '',
          },
        });
        return url;
      },
    },
  };
</script>
<style>
  .video-js.vjs-fluid:not(.vjs-audio-only-mode) {
    min-height: 100%;
  }
  .cld-video-player.cld-fluid {
    height: 100%;
  }
  .video-js .vjs-tech {
    object-fit: cover;
  }
</style>
@billnbell3 billnbell3 added the enhancement New feature or request label Sep 25, 2024
@Baroshem
Copy link
Collaborator

Hey there!

Thanks for reporting this issue. I do agree that it would be useful to have it on the component level. Would you be interested in contributing to the module with this change?

I can provide all the help needed to make it live :)

@colbyfayock
Copy link
Collaborator

hey wanted to chime in here because part of my goal with Next.js, the cousin to this library, is to have a similar API To the CldImage component as far as transformations go

I wasn't sure if the underlaying options for the Cloudinary Video Player supported this capability but I found that it appears to:

https://github.com/cloudinary/js-url-gen/blob/master/src/types/types.ts#L433

so technically speaking, you could as of now, pass in transformation={{ raw_transformation: 'string' }} and it would work

image

however I think it should be exposed as a top level option similar to CldImage from that consistency POV

to allow this library, Next, Svelte, and Astro to all benefit from this together, I would love to see this change added to our core library, Cloudinary Util: https://github.com/colbyfayock/cloudinary-util

whether one of you handle it, i handle it, or perhaps someone handles it through Hacktoberfest, I set up a new issue

cloudinary-community/cloudinary-util#201

@colbyfayock
Copy link
Collaborator

@billnbell3 aside from this feature support - i would be curious about what your use case is for needing raw transformations and if there isn't a specific transformation that we need to add support for?

@billnbell3
Copy link
Author

billnbell3 commented Sep 25, 2024 via email

@billnbell3
Copy link
Author

billnbell3 commented Sep 25, 2024 via email

@colbyfayock
Copy link
Collaborator

yeah sorry for the confusion, i was testing in the nextjs equivalent: https://next.cloudinary.dev/cldvideoplayer/basic-usage

@billnbell3
Copy link
Author

billnbell3 commented Sep 25, 2024

It actually does not work in VueJs. Probably needs encoding fix or something.

This is what I tried - no fading is in the URL when using this.

<CldVideoPlayer
    ref="videoPlayerRef"
    :src="videoId"
    autoPlay="always"
    :playsinline="true"
    :quality="quality"
    :fetch-format="format"
    controlsList="nodownload"
    :config="{ autoplay: true }"
    :transformation="{ transformation_raw: 'e_fade:2000,e_fade:-2000' }"
  /> 

@colbyfayock
Copy link
Collaborator

can you try

    :transformation="{ raw_transformation: 'e_fade:2000,e_fade:-2000' }"

had the naming backwards

@billnbell3
Copy link
Author

yep that works. The docs need upgrading. And the raw_transformation should be matching to other libraries.

@Baroshem
Copy link
Collaborator

@colbyfayock do you think these raw_transormation should be added as a top level prop? Or just added to the docs that it can be used inside transformation prop?

@colbyfayock
Copy link
Collaborator

however I think it should be exposed as a top level option similar to CldImage from that consistency POV

yeah my goal is to make the API between the video player and image component consistent, so it woudl make sense as a top level prop. ensuring the player options are documented and typed are important for a stop-gap solution and also for the other options that are already available. i have a separate ticket open to update the types

cloudinary-community/cloudinary-util#202

@colbyfayock
Copy link
Collaborator

Caveat: unless we decide to do a CldVideo component which I've considered in the past, the idea being it wraps the native <video element instead of bringing in the Video Player. Less customization and features, but if people dont care about that, they can ship less JS, so having that option could be beneficial. i still think we should make some APIs consistent, like rawTransformations as it makes sense, but the API for CldVideo could be almost identical to CldImage

@Baroshem
Copy link
Collaborator

Baroshem commented Oct 11, 2024

I see thanks for the comment @colbyfayock and sorry for long response. This comment of your completely disapeard from my notifications.

I would be up for adding this raw Transformations to the prop dearation of the component to support the same as useCldVideoUrl for now while in the background we could think about building CldVideo.vue component.

Would you like me to create an RFC based on this issue? :)

@billnbell3 would you be up for contributing to the module and adding these rawtransformations to CldVideoPlayer? I can provide all help needed :)

@colbyfayock
Copy link
Collaborator

if you think the CldVideo component would be valuable let's do it!

@billnbell3
Copy link
Author

I have no time to do this right now.

@Baroshem
Copy link
Collaborator

Baroshem commented Nov 5, 2024

@colbyfayock

I have added a note about raw transformations to CldVideoPlayer docs. For the thing about CldVideo component, I think first we would need to have an RFC about this component, gather feedback about how it should look like and behave. I will talk about it with Sanjay :)

@Baroshem Baroshem mentioned this issue Nov 5, 2024
Baroshem added a commit that referenced this issue Nov 5, 2024
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.

3 participants