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

New Adapter: Oraki #11727

Merged
merged 6 commits into from
Jun 26, 2024
Merged

New Adapter: Oraki #11727

merged 6 commits into from
Jun 26, 2024

Conversation

BenOraki
Copy link
Contributor

@BenOraki BenOraki commented Jun 7, 2024

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

DOC

prebid/prebid.github.io#5403

}
}

function getPlacementReqData(bid) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do 21 bid adapters use this exact same code block. Is one person committing twenty bid adapters?

If so, can you start to move code blocks into to libraries for better maintenance?

});
return bidFloor.floor;
} catch (err) {
logError(err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do not log an error when the floors module doesn't exist

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

A few things to change

},

buildRequests: (validBidRequests = [], bidderRequest = {}) => {
// convert Native ORTB definition to old-style prebid native definition
Copy link
Collaborator

@patmmccann patmmccann Jun 7, 2024

Choose a reason for hiding this comment

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

This is a utility function to make old code usable, a new module shouldn't need it

try {
refferLocation = refferUrl && new URL(refferUrl);
} catch (e) {
logMessage(e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If your test bid requests don't have proper locations this pollutes our circleci output a bunch

Copy link
Collaborator

Choose a reason for hiding this comment

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

see
image
in your circleci output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please point out the code line that is causing this problem?

let location = refferLocation || winLocation;
const language = (navigator && navigator.language) ? navigator.language.split('-')[0] : '';
const host = location.host;
const page = location.pathname;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with the page property from the request

}

let location = refferLocation || winLocation;
const language = (navigator && navigator.language) ? navigator.language.split('-')[0] : '';
Copy link
Collaborator

@patmmccann patmmccann Jun 7, 2024

Choose a reason for hiding this comment

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

Please don't get navigator language, use the request

try {
const winTop = window.top;
deviceWidth = winTop.screen.width;
deviceHeight = winTop.screen.height;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This creates a performance problem

@patmmccann
Copy link
Collaborator

please merge in latest master as well

@BenOraki
Copy link
Contributor Author

@patmmccann
Could you please review the last two commits that contain fixes for your comments?

@BenOraki BenOraki requested a review from ChrisHuie June 18, 2024 09:58
@patmmccann
Copy link
Collaborator

@BenOraki please take a look at #11854

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. Keep up the great work! 🚀

@BenOraki BenOraki requested a review from patmmccann June 25, 2024 09:06
@patmmccann patmmccann merged commit c533b08 into prebid:master Jun 26, 2024
4 of 5 checks passed
DecayConstant pushed a commit to mediavine/Prebid.js that referenced this pull request Jul 18, 2024
* New Adapter: Oraki

* added some fixes

* removed logError

* used lib functions to remove code duplication
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.

3 participants