Inject creativeComment after render so it actually persists #6860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smenzer
added
bugfix
needs 2nd review
jsnellbaker
approved these changes
Jun 9, 2021
|
@jsut can these conflicts be resolved so this can be merged? |
|
@mmoschovas rebased on master |
1 task
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Description of change
pbjs.renderAd currently injects an HTML comment into the body tag of the document passed in. This comment seems to get eaten up in any case where an ad is actually rendered successfully. Some bid adapters insert a comment similar to this in their renderers, but some don't. This change moves the actual injection of the comment to the HTML tag of the document, as well as changing it to run after the ad has been rendered. In my testing this actually results in a comment that you can see in the DOM for the adapters I happened to get demand from during manual testing.
I suppose there could/should be some tests for this, but i haven't written any since there aren't any renderAd tests that care about this comment. I'd be happy to look into getting some working if desired.