-
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
Update removeAdUnit #3527
Update removeAdUnit #3527
Conversation
What's the use case here? |
modification of adUnits in single page applications. Eventually we should remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove package-lock.json from the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make a single method that accepts an array of adUnit codes as an arg. If empty it would remove all adUnits
src/prebid.js
Outdated
* Remove all adUnits from the $$PREBID_GLOBAL$$ configuration | ||
* @alias module:pbjs.removeAllAdUnits | ||
*/ | ||
$$PREBID_GLOBAL$$.removeAllAdUnits = function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm should we make this removeAdUnits([adUnitCodeArray])
? seems more flexible.
@mkendall07 made changes and updated description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
src/prebid.js
Outdated
return; | ||
} | ||
|
||
var adUnitCodes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer let.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@bretg fyi we decided to not add an extra method here but instead reuse the existing one. |
Type of change
Description of change
pbjs.removeAdUnit()
updated removeAdUnit so that if it is invoked with no arguments it will remove all add units, and it will also accept an array argument of addUnitCodes and will only remove them.