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

Load ads directly instead of pulling from iframe #215

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Conversation

msub2
Copy link
Contributor

@msub2 msub2 commented Jun 24, 2024

This PR changes the behavior of the SDK to pull the wrapper in to the embedding page directly instead of going through the cross-origin frame, which was impacting viewability metrics. It also no longer initializes prebid resources if the ad unit ID provided is not a valid v4 uuid, along with a slight change to the A-Frame tests to account for varying image src.

@@ -1,5 +1,13 @@
import { test, expect } from '@playwright/test';

function srcEvaluate(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🙏🏼

@limbofeather
Copy link
Contributor

Would be good to leave a note on how to update the WLE test projects in the SDK README.

Copy link
Contributor

@limbofeather limbofeather left a comment

Choose a reason for hiding this comment

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

Small nit but not blocker. Looking forward to impact 🍏

document.head.appendChild(gifscript);

// Select baseDivId based on format, defaulting to the one for medium rectangle
if (format == 'billboard') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could all of this information on each format's attributes be in utils/formats.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably could yeah, can do in a small followup if this deploys without issue

@msub2
Copy link
Contributor Author

msub2 commented Jun 26, 2024

Would be good to leave a note on how to update the WLE test projects in the SDK README.

I've got an existing issue at #206 which would remove the need to manually build the test project, just haven't gotten around to figuring out the workflow yet

@msub2 msub2 merged commit 387333f into main Jun 26, 2024
1 check passed
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.

2 participants