Contxtful Rtd Provider : add ad unit positions #12792
Conversation
|
Tread carefully! This PR adds 1 linter error (possibly disabled through directives):
|
sebastienrufiange
changed the title
ChrisHuie
changed the title
| return elementCss.display !== 'none'; | ||
| } | ||
|
|
||
| function getElementFromTopWindowRecurs(element, currentWindow) { |
There was a problem hiding this comment.
These are very expensive layout calculations, are there similar calculations you might be able to import or reuse
There was a problem hiding this comment.
Thank you @patmmccann for the review!
That's a good point -- Actually the first thing we did is to seek for such reusable functions, but we discovered that many RTD and bid adapters implement their own ways, with different variations.
In particular, many uses the getBoundingClientRect function.
Concerning the approach here, Adagio RTD also does the same thing with the getElementFromTopWindow function.
There was a problem hiding this comment.
please see #12848 ; curious for your feedback
There was a problem hiding this comment.
Thank you @patmmccann for highlighting PR #12848; it’s a great starting point for enhancing reusability.
While it addresses a core aspect of ad position retrieval, additional work is needed for modules like ours to reduce more than just a few lines of code. I’d prefer not to block the merging of this PR while we wait for the other one, but if you believe we can get both merged in the next release, I can make the necessary changes.
If merging the other PR would delay things too much, could we proceed with this one? I’d be happy to assist with the other PR or create a new one to adapt after both are merged and available.
Thanks a lot!
Type of change
Description of change
This PR adds support for passing ad unit positions using ortb2Imp