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

Send placeholders for configured native assets #3573

Merged
merged 4 commits into from
Feb 27, 2019

Conversation

matthewlane
Copy link
Collaborator

@matthewlane matthewlane commented Feb 20, 2019

Type of change

  • Feature

Description of change

Transmitting some types of native assets via querystring parameters can cause issues. For example, large strings may exceed an adserver's url call limit, and special characters can get mangled. This PR introduces a configuration option to send placeholder values in adserver targeting, instead of asset values. The native creative template can then replace the placeholder values with the actual values through a postmessage request to Prebid (a PR to the native functionality in prebid-universal-creative will do this automatically).

Given the configuration:

pbjs.addAdUnits({
  code: slot.code,

  mediaTypes: {
    native: {
      body: {
        required: false,
        sendId: true
      },
      clickUrl: {
        required: true,
        sendId: true
      },
    },
  },

  bids: [...]
});

The body and clickUrl assets will be sent as placeholder values hb_native_body:<adId> and hb_native_linkurl:<adId>. All other native assets will be sent normally.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Largely LGTM; just made a note about the new field name in the native mediaTypes.

Also we need to get the docs changes put together.

src/native.js Outdated
@@ -163,10 +163,41 @@ export function getNativeTargeting(bid) {
value = value.url;
}

const sendPlaceholder = deepAccess(
bidReq,
`mediaTypes.native.${asset}.usePlaceholder`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any new thoughts on the name for this property? I know we were throwing other ideas around in a different discussion.

cc @mkendall07

@mkendall07
Copy link
Member

yeah good call on docs. @matthewlane can you get the doc PR up today?

@matthewlane
Copy link
Collaborator Author

Docs PR: prebid/prebid.github.io#1175

@matthewlane matthewlane added LGTM needs 2nd review Core module updates require two approvals from the core team and removed needs docs labels Feb 26, 2019
Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants