-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Renderer method used on all mediaType #3231
Comments
@jdelhommeau pbjs.onEvent("bidWon", function(bid) {
if (/* condition */) {
bid.renderer = {
url: rendererUrl,
render: rendererFn
};
}
}); |
Any updates here? This is a pretty big issue with anyone that wants to run a multi-format placement with banner + outstream video. @benjaminclot not sure the workaround works 100% of the time. In my case the renderer URL did not load. |
@pkayfire I use a fake URL as |
Here is possible solution to this bug: Prebid core installs renderer on bid response if renderer is present on adUnit. See here We can add renderer property inside This will not be a breaking change, as we will support existing feature as well where renderer is directly added at the topmost level in adUnit. Maybe we can remove that in 3.0 @mkendall07 @bretg Your thoughts. |
what if we just associated a renderer with every bid, and for banner the renderer is simply the "renderAd" function that is standard for banner? |
I really like @jaiminpanchal27 idea of having ability to assign renderer per mediaType. renderer for display: function(bid){ if (bid.width == 1800 and bid.height == 1000) { // skin case That would be a huge improvement over current way of implementing such format. |
For the sake of speed it could make sense to go with the solution by @mkendall07 and have the renderer per mediaType be a follow-up feature. |
Multiformat auctions still can't be used with custom renderer? |
@pkayfire and @mkendall07 was there any updates on this ticket? Has support for different renderer per mediaType been improved? |
So if a publisher were to define types in the ad unit renderer like this
and https://github.com/prebid/Prebid.js/blob/master/src/auction.js#L515 was modified from
would that work? |
plan is to implement the types check after #5638 gets merged so as to not introduce a merge conflict |
Not sure if there's a use case for this - but what if you also needed a separate renderer for display or native (or both)? I was thinking the publisher could define it like this, and it could potentially cover additional use cases:
In which case, I think you could do something like this:
Also, I do actually need this feature (either your proposed solution or this one would work for me)... so if you're not already working on this, I'd be happy to create a PR for this |
having up to three different renderers on a given ad unit, one for each mediatype, is a much better solution than the type check i am proposing, but we don't want the perfect to be the enemy of the good and that solution is much harder. |
Could a temporary fix (until the type check is implemented and merged) be something like just passing the bid through and checking if the mediaType is video in isRendererRequired? Line 100 in 466b49e
|
@patmmccann I actually think I got something that might work - it seems to be working so far. I can create a PR, or first paste a link to it in my fork when it's ready. |
Awesome, what have you got
…On Fri, Sep 11, 2020, 2:52 PM Mike Sperone ***@***.***> wrote:
@patmmccann <https://github.com/patmmccann> I actually think I got
something that might work - it seems to be working so far. I can create a
PR, or first paste a link to it in my fork when it's ready.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3231 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAM25Z2FKRPNKR5TV2BRFSDSFJWYTANCNFSM4F7KKG5A>
.
|
@patmmccann I want to add a test case still, but this is working for me - and the existing tests still pass
|
I'm not sure if I should make a new issue or ask this here, but I am trying to write a test for my above change, and placing my test after the 2 tests here: https://github.com/prebid/Prebid.js/blob/master/test/spec/auctionmanager_spec.js#L721-L764 I'm confused about the |
@MikeSperone - there's not really such a thing as a mediatype called The one place the video-outstream string exists is in the mediatypepricegranularity config, and that's a hack shortcut. |
Thanks @bretg! That was very helpful! I found out that my test was actually working, it was my code that had the error |
I think this was solved by 5760 and can be closed |
Solved with #5760 |
Type of issue
Bug
Description
When running multiple mediaType on a single adUnit (for example: outstream + banner) and you need to use client side renderer for the video outstream, the renderer will also try to render banner ad, not just outstream ad. Ideally, there is a way to either have a way in renderer to fallback on default renderer, OR the renderer is only called for video ad returned.
Steps to reproduce
1 - Build an adUnit for video and banner mediaType that delivers both video and banner creative
2 - define manual renderer
3 - run. When a banner ad is returned, the rendering fails because of missing information (bid.adResponse is missing on banner)
Test page
http://jdelhommeau.devnxs.net/POC/VIDEO/prebid_outstream_banner_plct.html
Expected results
Way to fallback on default renderer when in renderer method OR renderer is only used when video mediatype is returned
Actual results
Renderer is called all the time for all mediaType and fails when not video
Platform details
Other information
The text was updated successfully, but these errors were encountered: