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

[Criteo Adapter] Use utils lib to parse request sizes #1469

Merged
merged 1 commit into from
Aug 10, 2017

Conversation

Swiiip
Copy link
Contributor

@Swiiip Swiiip commented Aug 3, 2017

Type of change

  • [x ] Bugfix

Description of change

This PR formats the request sizes in Criteo adapter. As those sizes are not formatted, we need to format them before using them in a consistent manner.

@Swiiip Swiiip force-pushed the criteo-parseSize branch 2 times, most recently from 982beb1 to 40a201f Compare August 3, 2017 14:24
@mkendall07 mkendall07 assigned dbemiller and unassigned mkendall07 Aug 7, 2017
@mkendall07 mkendall07 added the LGTM label Aug 7, 2017
var xIndex = sizeString.indexOf('x');
var w = parseInt(sizeString.substring(0, xIndex));
var h = parseInt(sizeString.substring(xIndex + 1, sizeString.length))
return new Criteo.PubTag.DirectBidding.Size(w, h);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What effect has this change? We are currently sending all our ad slot sizes,
but were told that the backend doesn't use them anyway. Will this change
in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will change in the future, we will need this size information to handle the request on our end.

@dbemiller dbemiller merged commit ee03c84 into prebid:master Aug 10, 2017
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants