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

Support multiformat ad units #811

Merged
merged 31 commits into from
Apr 14, 2023

Conversation

OlenaPostindustria
Copy link
Collaborator

@OlenaPostindustria OlenaPostindustria commented Apr 4, 2023

Closes #809

@OlenaPostindustria OlenaPostindustria force-pushed the feature/support-multiformat-ad-units branch from eebd01b to b50b9eb Compare April 4, 2023 14:19
@OlenaPostindustria OlenaPostindustria marked this pull request as ready for review April 6, 2023 13:36
Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

Please address several minor comments.

public var adFormats: Set<AdFormat> {
get { adUnitConfig.adFormats }
set { adUnitConfig.adFormats = newValue }
}

public init(configId: String, size: CGSize) {
super.init(configId: configId, size: size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the adFormats an obligatory property of AdUnit initializer so we are sure that this property will never be undefined?

var bannerParameters: BannerParameters { get set }
}

@available(*, deprecated, message: "This class is deprecated.")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should inform publishers which class they should use instead of the deprecated one.

@@ -41,7 +41,7 @@ public class VideoParameters: NSObject {
* "video/mp4"
* "video/x-ms-wmv"
*/
public var mimes: [String]?
public var mimes: [String]
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put a comment on why the mimes can't be optional.

@@ -15,21 +15,24 @@ limitations under the License.

import UIKit

public class RewardedVideoAdUnit: VideoBaseAdUnit {
@objcMembers
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious - is it mean that this class, and interstitial ad unit as well, hasn't been used in Obj-c apps?

Copy link
Collaborator Author

@OlenaPostindustria OlenaPostindustria Apr 10, 2023

Choose a reason for hiding this comment

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

Actually, there is no need for this attribute in these classes. They inherit from AdUnit class, which already provides this attribute that also applies to subclasses and their properties/methods and makes them available in Obj-C apps. Removed it from AdUnit subclasses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see! Great!

return nil;
}

self.targeting = [PBMMutableJsonDictionary new];
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not super clear from the code, just, please make sure that targeting object is absent in the request if it doesn't have any internal properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sending this object is required for getting targeting keywords in response. More info here.

@@ -81,6 +81,10 @@ - (void)buildBidRequest:(nonnull PBMORTBBidRequest *)bidRequest {
bidRequest.extPrebid.cache = cache;
}

if (adFormats.count >= 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment on why we have this condition here and not just send the includeformat for all cases.

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

LGTM

@YuriyVelichkoPI YuriyVelichkoPI merged commit c36726e into master Apr 14, 2023
@YuriyVelichkoPI YuriyVelichkoPI deleted the feature/support-multiformat-ad-units branch April 14, 2023 11:50
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.

Support multiformat adunits
2 participants