-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Mobile refactoring: adops focus #5507
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for prebid-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the review is still in progress. I'm just committing comments once they arise
prebid-mobile/recipes/subrecipes/ios/gam-bidding-only-html-banner.md
Outdated
Show resolved
Hide resolved
prebid-mobile/recipes/subrecipes/android/gam-bidding-only-html-banner.md
Show resolved
Hide resolved
|
||
### Banners | ||
|
||
#### Display Banners | ||
|
||
Integration example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to choose and follow the same approach for introducing the code snippets. The code in iOS Bidding-Only doc has different formatting and include the language name:
We can address it in a separate scope because it can be challenging to find all the places in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone needs to review all the snippets anyhow for correctness and good comments. How about we make the intros consistent at that time?
|
||
#### Display Banners | ||
|
||
To integrate the banner ad you need to implement three easy steps: | ||
|
||
```kotlin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the future adjustments. We should use the same approach for creating the code snippets over the whole documentation.
In this particular case, the code snippet presents only some piece of code, but in most other cases snippets alsow include the code of the function which holds this code. Usually, we take all code snippets from the Demo Applications. So it shouldn't be a problem to add the whole function in the same way as it exists in the Demo App.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, but understanding the code is more than I'm willing to do here. We'll need a volunteer to deal with this part.
@@ -165,15 +153,14 @@ Pay attention that the `loadAd()` should be called on the main thread. | |||
|
|||
The **default** ad format for an interstitial ad is **DISPLAY**. In order to make a `multiformat bid request`, set the respective values into the `adUnitFormats` parameter. | |||
|
|||
``` | |||
```kotlin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not related to the changes in the PR, but I see inconsistency with the current code. The InterstitialAdUnit
doesn't have such initialization properties as minSizePercentage
. It is not in iOS or the Android SDK. It looks like a legacy code.
We can create separate ticket to address legacy code issues discovered in this PR.
We recommend that you study the flows documented in these different rendering approaches: | ||
|
||
- [Bidding-Only](/prebid-mobile/pbm-api/{{include.platform}}/{{include.platform}}-sdk-integration-gam-original-api.html#how-it-works) - generally this approach utilizes webviews and the Prebid Universal Creative to render most types of ads. | ||
- [Prebid-Rendered](/prebid-mobile/modules/rendering/{{include.platform}}-sdk-integration-gam.html) - with this approach the Prebid SDK will render any bids that are chosen for display. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach is irrelevant to the integration with Custom Server. Publishers can't just replace the GMA sd with a custom one. The flow in the "Prebid-Rendered" approach is relevant only to the GAM platform and its features. So, this item and lines 28-31 can mislead integrators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular page utilizes the same tiles in a different manner:
- Bidding only
- in the beginning of the doc it refers to the GAM SDK + PUC integration scenario
- then it refers to a scenario with a custom server that does not necessarily use PUC. The custom server can process the raw bid and render it's
adm
.
- Prebid Rendered
- in the beginning of the doc it refers to the GAM SDK + Prebid Event Handlers + Prebid SDK integration scenario
- then it refers to the Prebid SDK Rendering API.
It can confuse a reader a bit, too.
I think that we should skip the detailed description of GAM-based scenarios in this integration approach - ios-sdk-integration-pb page.
* `events` - the map of some publically available event URLs attached to the bid. These can be used to enable Prebid Server-based analytics when the Prebid Universal Creative (PUC) is not involved in the rendering process. If the PUC is used for rendering, it will take care of hitting these events. These are the available event URLs: | ||
* **EVENT_WIN** - this bid was chosen by the ad server as the one to display. This is the main metric for banner and native. This returns the OpenRTB `seatbid.bid.ext.prebid.events.win` field. (requires SDK v2.1.6) | ||
* **EVENT_IMP** - the ad creative for this bid was actually displayed. This is often the main metric for video ads. This returns the OpenRTB `seatbid.bid.ext.prebid.events.imp` field. (requires SDK v2.1.6) | ||
- `resultCode` - the object of type `ResultCode` describing the status of the bid request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is better to say "the status of the bid response".
Some sections that are ready for review: