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

Pass the adUnit array when emitting SET_TARGETING #1875

Closed
wants to merge 1 commit into from

Conversation

ptomasroos
Copy link
Contributor

We use this in our analytics adapter very useful to have as args

  • Feature

Description of change

Since the auction process is detached from the DFP rendering process aka when does a call against DFP happen we find it very useful in our analytics adapter to have which ad unit's or all are getting invoked

We use this in our analytics adapter very useful to have as args
@@ -230,7 +230,7 @@ $$PREBID_GLOBAL$$.setTargetingForGPTAsync = function (adUnit) {
targeting.setTargeting(targetingSet);

// emit event
events.emit(SET_TARGETING);
events.emit(SET_TARGETING, adUnit);
Copy link
Member

Choose a reason for hiding this comment

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

adUnit is optional here. Probably better to define some structure. Is this an array, object etc?

Copy link
Member

Choose a reason for hiding this comment

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

Once that is done, it should be applied to the other SET_TARGETING emitter - see line 250

Copy link
Member

@mkendall07 mkendall07 Nov 27, 2017

Choose a reason for hiding this comment

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

Just to be clear, my preference is something like:

events.emit(SET_TARGETING, { adUnits: [adUnit1, adUnitN...]});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. Will fix!

@mkendall07
Copy link
Member

@ptomasroos
any chance to work on this?

@ptomasroos
Copy link
Contributor Author

Yes will fix tomorrow GMT time @mkendall07 forgot

@bretg
Copy link
Collaborator

bretg commented Feb 2, 2018

@ptomasroos - any update?

@mkendall07
Copy link
Member

closing as no update in several months.

@mkendall07 mkendall07 closed this Feb 13, 2018
@ptomasroos ptomasroos mentioned this pull request May 22, 2018
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants