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

Pbm native #247

Merged
merged 34 commits into from
Dec 9, 2019
Merged

Pbm native #247

merged 34 commits into from
Dec 9, 2019

Conversation

ppuviarasu
Copy link
Collaborator

@ppuviarasu ppuviarasu commented Oct 14, 2019

Implementation of PrebidNative for bannerNative DFP & mopub.

This is a banner native implementation as the creative is rendered as a universal creative served on a webview

@ppuviarasu ppuviarasu requested review from yoalex5 and removed request for akashVermaCardinal October 28, 2019 19:25
* Added Unittest cases for PBM Native

* Test cases updated
Copy link
Collaborator

@yoalex5 yoalex5 left a comment

Choose a reason for hiding this comment

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

Please review PR #254

Also expose the new API for Objective-C

@yoalex5
Copy link
Collaborator

yoalex5 commented Nov 8, 2019

As I see you use https://acdn.adnxs.com/prebid/test/jp/native-trk.js to render native ad.
Does it mean that we are not going to extend PUC for this purpose ?

@yoalex5
Copy link
Collaborator

yoalex5 commented Nov 11, 2019

#255 fixes modularisation issue

Copy link
Collaborator

@yoalex5 yoalex5 left a comment

Choose a reason for hiding this comment

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

iOS and Android versions have different API for a publisher
Can we align this ?

Source/AdUnits/Native/NativeRequest.swift Show resolved Hide resolved
Source/AdUnits/Native/NativeRequest.swift Show resolved Hide resolved
Source/AdUnits/Native/NativeAsset.swift Outdated Show resolved Hide resolved
public var ext: AnyObject?

public required init(minimumWidth: Int, minimumHeight: Int, required: Bool) {
super.init(isRequired: required)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the publisher wants just exact width and height, but not mW and mH?
Can you provide an alternative Init method?

@objc public enum ImageAsset: Int {
case Icon = 1
case Main = 3
case Custom = 500
Copy link
Contributor

Choose a reason for hiding this comment

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

This Custom can not be changed?

public var length: Int?
public var ext: AnyObject?

public required init(type: Int, required: Bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Data has 12 fixed types and one custom type. Should not be using Int.

Copy link
Contributor

@anwzhang anwzhang left a comment

Choose a reason for hiding this comment

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

Missing support for MoPub ad resizing.

@anwzhang
Copy link
Contributor

Hi @yoalex5,
For your question:

As I see you use https://acdn.adnxs.com/prebid/test/jp/native-trk.js to render native ad.
Does it mean that we are not going to extend PUC for this purpose ?

Source code is in PUC, please check the latest release: https://github.com/prebid/prebid-universal-creative/releases
however, the native part code will be hosted here: https://cdn.jsdelivr.net/npm/prebid-universal-creative@latest/dist/native-trk.js
Since there're concerns about PUC getting larger and larger. Please test if you need it.

@ppuviarasu ppuviarasu dismissed yoalex5’s stale review December 9, 2019 17:44

Wei will complete the review for release

anwzhang
anwzhang previously approved these changes Dec 9, 2019
@ppuviarasu ppuviarasu merged commit 6d9a0d4 into master Dec 9, 2019
@ppuviarasu ppuviarasu deleted the PBM_Native branch December 9, 2019 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants