Add support for multiple sizes #5626
Conversation
| 'sizes': [[0, 1], [2, 3], [4, 5], [6, 7]] | ||
| }; | ||
|
|
||
| expect(spec.isBidRequestValid(bid)).to.equal(true); |
There was a problem hiding this comment.
I do not believe this tests the code change.
You'll need to probably build the requests spec.buildRequests and validate data.bf is present and looks correct.
There was a problem hiding this comment.
added a new unit test for this; thanks for calling that out :)
modules/gumgumBidAdapter.js
Outdated
| @@ -275,6 +275,9 @@ function buildRequests (validBidRequests, bidderRequest) { | |||
| } | |||
| if (params.inSlot) { | |||
| data.si = parseInt(params.inSlot, 10); | |||
| if (params.sizes) { | |||
| data.bf = params.sizes.map((i) => `${i[0]}x${i[1]}`) | |||
There was a problem hiding this comment.
If this is a new thing, if you could please add this at least to your .md and make sure it is known it MUST be an array of size arrays like so: [[0, 1], [2, 3], [4, 5], [6, 7]]
Else, make this code catch bad input better so no exceptions are thrown?
There was a problem hiding this comment.
added to md file and catching bad input better
There was a problem hiding this comment.
Not sure you actually pushed the commits for this yet.
I do not see the md file update or any logic change.
There was a problem hiding this comment.
sorry, just pushed again
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Please update PR in order to keep it from auto closing. |
estavillo
force-pushed
the
AT-6613-gumgum-multiple-sizes
branch
4 times, most recently
from
79cb509
to
62e4ad0
Compare
modules/gumgumBidAdapter.md
Outdated
| @@ -25,6 +25,18 @@ var adUnits = [ | |||
| } | |||
| } | |||
| ] | |||
| },{ | |||
| code: 'test-div', | |||
| sizes: [[0, 1], [2, 3], [4, 5], [6, 7]], | |||
There was a problem hiding this comment.
This code change seems to be taking from params.sizes not adUnit.sizes
So I think the md file should have this inside the gumgum bidder params as an option.
There was a problem hiding this comment.
If not intended to be in your markdown file, maybe it should be in the prebid docs?
Not required to have it here so I will still merge.
There was a problem hiding this comment.
Correct; I removed this for now
| }; | ||
| sizes | ||
| const bidRequest = spec.buildRequests([request])[0]; | ||
| expect(bidRequest.data.bf).to.equal('0x1,0x2'); |
|
Looks like there are some test issues. Please take a look and fix them |
| 'inSlot': '123', | ||
| 'sizes': [[0, 1], [0, 2]] | ||
| }; | ||
| sizes |
There was a problem hiding this comment.
Think you need to remove this
add UT UT
estavillo
force-pushed
the
AT-6613-gumgum-multiple-sizes
branch
from
62e4ad0
to
d79bc79
Compare
add UT UT Co-authored-by: Estavillo <fernando@gumgum.com>
This reverts commit 735a239.
Type of change
Description of change
Added support for multiple slot sizes
Be sure to test the integration with your adserver using the Hello World sample page.
For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:
Other information