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

ME-14380-lazy-ads #535

Merged
merged 4 commits into from
Feb 5, 2024
Merged

ME-14380-lazy-ads #535

merged 4 commits into from
Feb 5, 2024

Conversation

tsi
Copy link
Collaborator

@tsi tsi commented Feb 5, 2024

No description provided.

@tsi tsi self-assigned this Feb 5, 2024
Copy link
Contributor

@ehab-cl ehab-cl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💪

@@ -67,7 +67,6 @@ <h3 class="mb-4">VAST and VPAID</h3>
playsinline
controls
muted
autoplay
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the non autoplay was causing a bug it seems that autoplay can be a factor, i would provide two examples one with autoplay and one without.

Comment on lines +1 to +3
import 'videojs-contrib-ads';
import 'videojs-ima';
import 'videojs-ima/dist/videojs.ima.scss';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This versus having the library code getting copied is 🕺 🚀

@@ -38,11 +36,11 @@ export default function imaPlugin(player, playerOptions) {
if (Object.keys(playerOptions.ads).length > 0 && typeof player.ima === 'object') {
if (playerOptions.ads.adsInPlaylist === 'first-video') {
player.one(PLAYER_EVENT.SOURCE_CHANGED, () => {
player.ima.playAd();
player.ima.playAdBreak();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between these two methods? just curious or is it just a rename?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look too deep into the history of the change, but looks like it was renamed

Copy link

netlify bot commented Feb 5, 2024

Deploy Preview for vp-build ready!

Name Link
🔨 Latest commit 0095553
🔍 Latest deploy log https://app.netlify.com/sites/vp-build/deploys/65c0b192d350960008b99cab
😎 Deploy Preview https://deploy-preview-535--vp-build.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tsi tsi merged commit 981804d into edge Feb 5, 2024
6 checks passed
@tsi tsi deleted the ME-14380-lazy-ads branch February 5, 2024 10:10
This was referenced Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants