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

InSkin Bid Adapter: add user syncing #2287

Merged
merged 1 commit into from
Apr 3, 2018

Conversation

jgrimes
Copy link
Contributor

@jgrimes jgrimes commented Mar 20, 2018

Type of change

  • Other

Description of change

Adds in several user sync URLs for image and iframe based user syncing.

@jgrimes jgrimes force-pushed the inskin-user-sync branch 2 times, most recently from a50e166 to 0131f3d Compare March 20, 2018 01:40
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.

Hello @jgrimes ,

These changes do LGTM, however I did notice something that I wanted to run by you.

While testing on the hello_world page with your adapter's test data, I saw the pixel URL for https://e.serverbid.com/ was being loaded twice per page load. When I specifically disabled the pixel userSync via the config options, the URL was still there (I assume it's part of the ad?).

Is this expected? I was concerned it may double your tracking metrics. Please let me know when you can.

@jsnellbaker
Copy link
Collaborator

Hello @jgrimes ,

When you have the chance, can you review the question above? The PR looks good, but I wanted to confirm with you on this outstanding point. Thanks.

@jsnellbaker
Copy link
Collaborator

@jgrimes I'm going to merge this PR at this time. If you need to change the userSync pixels, please submit a new PR.

@jsnellbaker jsnellbaker merged commit 61c0bee into prebid:master Apr 3, 2018
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
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.

2 participants