-
Notifications
You must be signed in to change notification settings - Fork 31
Cloudinary video player does not conform to transformation string set in the settings #42
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
Conversation
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.
Hi @dugajean,
Nice work, this required a lot of digging into the API, it looks like.
If you agree with the main point of #42 (comment), the rest of the comments aren't really relevant.
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
kienstra
left a comment
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.
Hi @dugajean,
Nice work, this is a tough bug.
It'd be nice if we could avoid the code duplication in the script. You've looked at this more, so you could have a much better approach than me.
|
@kienstra I'll do that then! 👍 |
|
@kienstra Implemented your suggestion from above. Works flawlessly! Also kept the controls check removed as you noted above and let it up to the user to remove the controls if they see it as necessary. |
kienstra
left a comment
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.
Hi @dugajean,
Just some minor points here. Also, I'll do a quick test of this in a bit.
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/media/class-video.php
Outdated
Show resolved
Hide resolved
kienstra
left a comment
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.
Thanks, almost there.
|
@kienstra Should be all good now! 🎉 |
kienstra
left a comment
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.
Approved
Hi @dugajean,
Thanks for your patience, and for staying so late today. This looks good.
|
Feel free to merge this when you'd like. |
|
I'm merging this, but if there's something you'd like to change, feel free to open another PR. |
After a lot of research on this bug and a lot of failed attempts, I'm finally happy with my solution.
As Ryan has described, steps to reproduce:
The Cloudinary video player JS library has a ton of options that can be added when instantiating it (including
widthandheight), but simply parsingw_100andh_100into an options object and passing it didn't seem like a very scalable solution as there are a lot more transformations that could be applied.I realized that transformations could be directly added to the URL in their short form (ie. w_100), so parsing the transformations is no longer needed.
Example:
Turns into:
I generate a small amount of JS to switch the URL for each Cloudinary video on the page and apply the global transformations accordingly.
Result: